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

Fixed linting local development #606

Merged

Conversation

mokshith-moksh
Copy link
Contributor

Linked Issue(s)

Closes #600

Acceptance Criteria fulfillment

  • The linter should work locally on respective submodules.
  • The linter should run on saving of files (only the currently changed file(s) ideally).
  • It should work on respective files regardless of whether I opened the submodule directly or the whole repo.
  • It should work at least in VSCode perfectly.

Proposed changes (including videos or screenshots)

Further comments

The solution involved configuring the ESLint settings in both the CLI and web-server submodules to ensure consistent behavior across the repository. I prioritized getting it to work seamlessly in VSCode, as specified in the acceptance criteria. Alternative approaches were considered, but this configuration best meets the project's linting requirements.

@mokshith-moksh
Copy link
Contributor Author

there was the error at e051571 because of the change in prettier version , I have updated it , now it's perfectly Good.

cli/yarn.lock Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your yarn install updated some things here. Try yarn install with --frozen-lockfile

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am working on it

cli/package.json Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly changed in this file and why?

Copy link
Contributor Author

@mokshith-moksh mokshith-moksh Oct 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jayantbh ,

In the pre-commit.yml file, testing is occurring using the command: yarn add eslint@^8.40.0 [email protected] eslint-plugin-import@^2.29.0 eslint-plugin-prettier@^5.0.1 eslint-plugin-react@^7.29.4 eslint-plugin-unused-imports@^3.0.0 --dev. I thought these packages need to be installed in the cli app to ensure that linting works properly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No no. I mean, you see the whitespace changes right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking if you've seen this reply. ☝🏽

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes in this file seem unintentional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jayantbh,

I added this because the ESLint rule configuration for quotes ensures that the code follows consistent quote usage. The settings are as follows:

  • error : This means ESLint will throw an error if the rule is violated.
  • single : This enforces the use of single quotes for strings.
  • { avoidEscape: true, allowTemplateLiterals: true }`:
    • avoidEscape: true allows the use of double quotes if it helps avoid escaping single quotes, making the code
      more readable.
    • allowTemplateLiterals: true permits the use of template literals (backticks) for strings that require interpolation or multi-line formatting.

This configuration is designed to enforce consistent and readable string formatting throughout the codebase.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was pointing to the abundance of blank lines

Copy link
Contributor

@jayantbh jayantbh Oct 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which seem to be gone now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jayantbh,

I apologize for the issue. I have fixed all the problems. Could you please go through it now?

@jayantbh
Copy link
Contributor

Hey @mokshith-moksh, I recommend renaming the commits to better reflect that they actually contain.

@@ -27,6 +27,11 @@ module.exports = {
}
},
rules: {
quotes: [
'error',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think error should be thrown around like that.

Things that can potentially lead to serious or breaking outcomes should be set to the error level, but otherwise should be set at a warning level.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jayantbh,
I gave changed to warn , the linter will still detect violations (like using double quotes), but instead of failing, it will issue a warning

@jayantbh
Copy link
Contributor

I see that there's a bunch of changes to the configs, but I'd expect for a bunch of code files to also have changed assuming the rules were actually applied and run on them as a result.

Nothing changed?

I mean, it is not impossible given that the linting check passed successfully.

@mokshith-moksh
Copy link
Contributor Author

mokshith-moksh commented Oct 21, 2024

Hi @jayantbh,
fixed: change quotes rule from error to warn and applied linting across codebase , now there is couple changes in the codebase as linting is applied to the codebase.

@jayantbh
Copy link
Contributor

@mokshith-moksh I see a lot of previous commits with the same improper naming. Only the most recent one named appropriately. You might need to do an interactive rebase.

image

@mokshith-moksh mokshith-moksh force-pushed the Fixed-linting-local-development branch 2 times, most recently from fe6cfdb to e6863a4 Compare October 23, 2024 07:13
@mokshith-moksh
Copy link
Contributor Author

Hi @jayantbh,

I have performed an interactive rebase and renamed the commits with proper naming.

…ss codebase#600

fix: change quotes rule from error to warn and apply linting across codebase-middlewarehq#600
@mokshith-moksh mokshith-moksh force-pushed the Fixed-linting-local-development branch from 3a8e8d8 to ad6d1cc Compare October 23, 2024 08:12
Copy link
Contributor

@jayantbh jayantbh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work for me! Good stuff!

@jayantbh jayantbh merged commit 5740ba4 into middlewarehq:main Oct 23, 2024
3 checks passed
@mokshith-moksh mokshith-moksh deleted the Fixed-linting-local-development branch October 23, 2024 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix linting for local development
2 participants