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

[OSCI][Enhancement] precommit(husky) and tsconfig changes #931

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kohinoor98
Copy link
Contributor

@kohinoor98 kohinoor98 commented Nov 15, 2023

Description

Made changes to .eslintrc.js for defining lint rules, .lintstagedrc for pre-commit check and, eslint:* scripts added to package.json

Important
This PR must be merged after #930 is merged into the codebase. Developers cannot commit code due to multiple lint issues already addressed in #930.

Issues Resolved

#86

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: kohinoor98 <[email protected]>
@kohinoor98 kohinoor98 changed the title precommit and tsconfig changes precommit(husky) and tsconfig changes Nov 15, 2023
@kohinoor98 kohinoor98 changed the title precommit(husky) and tsconfig changes [OSCI][Enhancement] precommit(husky) and tsconfig changes Nov 16, 2023
Copy link

codecov bot commented Nov 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (be76a29) 63.37% compared to head (8840d15) 63.37%.

Additional details and impacted files
@@           Coverage Diff            @@
##             main     #931    +/-   ##
========================================
  Coverage   63.37%   63.37%            
========================================
  Files         341      341            
  Lines       11554    11554            
  Branches     2243     2112   -131     
========================================
  Hits         7322     7322            
  Misses       3658     3658            
  Partials      574      574            

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +28 to +39
"@osd/eslint/no-restricted-paths": [
"error",
{
basePath: __dirname,
zones: [
{
target: ["(public|server)/**/*"],
from: ["../../packages/**/*", "packages/**/*"],
},
],
},
],
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about the idea behind this rule, can you add a message that can explain, like here https://github.com/opensearch-project/OpenSearch-Dashboards/blame/791210de027467f95b13f7e0c9582fa149e08eb5/.eslintrc.js#L277

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bowenlan-amzn
The "@osd/eslint/no-restricted-paths" rule in the ESLint configuration was added to enforce module import restrictions within the codebase.
"@typescript-eslint/naming-convention" rule: it was added to enforce consistent naming conventions across the codebase. This rule specifies that all identifiers (like variables, classes, interfaces, etc.) should follow one of the defined formats: camelCase, UPPER_CASE, PascalCase, or snake_case. It also allows for leading and trailing underscores.

Please let me know if you want me to remove any/both of these rules

Copy link
Member

Choose a reason for hiding this comment

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

yes, for @osd/eslint/no-restricted-paths part, I suppose it's restricting import from packages in public or server. I am just not sure what's the ideo behind this. @kohinoor98

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@osd/eslint/no-restricted-paths: It prevents certain parts of your codebase (like public-facing or server-side code) from importing modules from restricted directories, which might be private and not intended for such use.
@bowenlan-amzn

package.json Outdated Show resolved Hide resolved
@SuZhou-Joe
Copy link
Member

Overall LGTM.

SuZhou-Joe
SuZhou-Joe previously approved these changes Nov 17, 2023
kohinoor98 added a commit to kohinoor98/index-management-dashboards-plugin that referenced this pull request Jan 10, 2024
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.

3 participants