Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add linting step to make ui for consistent code quality #1219

Open
Mindslayer001 opened this issue Dec 31, 2024 · 5 comments
Open

Add linting step to make ui for consistent code quality #1219

Mindslayer001 opened this issue Dec 31, 2024 · 5 comments
Labels
enhancement Enhancement request

Comments

@Mindslayer001
Copy link

Problem

While building the UI using the make ui command, the following sequence of commands is run:

@cd ui && pnpm pre-install && pnpm build && cd -

During development, we ran into an issue where linting was not done consistently. Specifically, I ran npm run lint, but other developers didn't, which made it harder to find and fix the issue, especially with Issue #1217.

Suggested Solution

To improve consistency and avoid future confusion, we should add the npm run lint step to the make ui process. This will ensure linting is always run as part of the build process.

Proposed Update:

ui:
    @cd ui && pnpm pre-install && pnpm build && npm run lint && cd -

This ensures that linting is always performed after building the UI, maintaining consistent code quality across all developers.

Alternatives Considered

  • CI/CD Pipeline Linting: Ensure linting is mandatory in the CI/CD pipeline to catch any linting issues before deployment.
  • Pre-commit Hooks: Use pre-commit hooks (e.g., Husky) to enforce linting before every commit, ensuring developers don’t forget to lint locally.

Impact

Adding this change will:

  • Ensure consistent linting across all developers.
  • Prevent hard-to-find bugs related to inconsistent code quality.
  • Make the development process smoother and more predictable.
@Mindslayer001 Mindslayer001 added the enhancement Enhancement request label Dec 31, 2024
Mindslayer001 added a commit to Mindslayer001/incubator-answer that referenced this issue Dec 31, 2024
@Mindslayer001
Copy link
Author

I want to solve this issue.

@shuashuai
Copy link
Member

It is meaningless to add lint in the build phase, because the build product will be compressed, and it is meaningful in the development and submission phase.

At the beginning, we ran lint through husky's pre-commit phase during the submission phase, so as to ensure the agreed code style. Later, the verification was canceled due to some other reasons.

@Mindslayer001
Copy link
Author

@shuashuai Thanks for the feedback! I understand that linting in the build phase may not be as useful since the build product is compressed.

However, adding linting to the build process ensures it's always run, even if developers forget to do it manually. I agree that Husky pre-commit linting is a good approach. Another idea could be to apply linting in the npm run start command, so it's automatically run during development without relying on developers to remember it.

What do you think?

@shuashuai
Copy link
Member

shuashuai commented Dec 31, 2024

@Mindslayer001

You can first withdraw this solution from the previous PR and submit a separate PR, so that it will not affect the submission of the previous PR.

I need to think again about how to add lint verification scheme when submitting code, because the method you mentioned above is not a standard solution, or see if other people in the community have better methods.

@Mindslayer001
Copy link
Author

@shuashuai ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement request
Projects
None yet
Development

No branches or pull requests

2 participants