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.

Sign up for GitHub

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

Jump to bottom

feat: implementation of pre commit hooks #27

Merged
merged 2 commits into from
Nov 12, 2024
Merged

feat: implementation of pre commit hooks #27

merged 2 commits into from
Nov 12, 2024

Conversation

Neha
Copy link
Contributor

@Neha Neha commented Oct 29, 2024 •
edited by giolaq
Loading

Description

Added husky pre-commit hook for:

  1. eslint
  2. prettier
  3. commit messages

Fixes #17

Snapshots/Videos:
Screenshot 2024-10-29 at 21 50 07
Screenshot 2024-10-29 at 21 49 50

Type of change

  • 🍕 Feature

Added tests?

  • 🙅 No, because they aren't needed

Internationalization Support?

  • 🙅 No, because they aren't needed

Steps to test the feature:

  1. run the project at local
  2. do some changes
  3. try to commit the changes without proper message.
  4. observe, this should throw an error
  5. do some formatting issues
  6. commit the changes
  7. observe, the formatting issues will be automatically fixed

Snapshot of the test-cases that are passing:
NA

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
NA

"By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice."

@Neha Neha changed the title Feat/implement pre commit hooks Feat: implement pre commit hooks Oct 29, 2024
@Neha Neha changed the title Feat: implement pre commit hooks feat: implementation of pre commit hooks Oct 30, 2024
@giolaq
Copy link
Contributor

giolaq commented Nov 1, 2024

Thank you!

Can you expand on the rules used and why are they useful? It's good to have this for the future .

Also why using husky? Can we use git hooks? What's the benefit of it?

@Neha
Copy link
Contributor Author

Neha commented Nov 1, 2024

Thank you!

Can you expand on the rules used and why are they useful? It's good to have this for the future .

Also why using husky? Can we use git hooks? What's the benefit of it?

prettier rules are the standard rules for managing the use of tabs, width, singlequotes, etc. In , pettierrc these rules are mentioned. As required in future we can add, or edit rules. These are useful to manage the consistency of the project's code formatting. In the absence of this, ever developer will be formatting the code as per their preference and this will leads to inconsistency, and in PR lot of unnecessary diff will get logged

ESLint rules have the react, typescript, react-native, and jsx-a11y. The few rules to start with are - file extension should be .tsx, no-inline-style, no unused var, etc. These rule are mentioned in file eslint.config.js. These rules can be edited in future. ESLint rules will make sure that developers are using the right and good pattern of coding as suggested by the React, TS, and RN. While coding if there would be any issue, ESLint will highlight. Eg: if the accessibility is missing , or a variable is not used but declared then while coding eslint will throw error or warning as per the rule and help the developer to do the needful.

Husky is flexible and simpler than git. With husky we just need package.json to setup, and commands. However with git the pre commit hooks installation is not easier. Another bigger advantage of husky over git is, husky will be installed for every developer however with git, every developer has to setup on their machine. So, all environments will be handle by the husky as compare to the git

giolaq
giolaq reviewed Nov 5, 2024
View reviewed changes
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.

Do we need this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it is yarn.lock we do need this for fix version packages

Copy link
Contributor

@giolaq giolaq Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So are you proposing to use yarn instead of npm? I don't get it in this PR. If this is the case we should also modify the readme and test it all, we can have discuss it in a different PR. We tried on #1 but the tvOS version didn't work

.eslintrc Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this file? Can we just use eslint.config.js?

components/MenuContext.tsx Outdated
{children}
</MenuContext.Provider>
const value = useMemo(
() => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

is this related to this PR?

Copy link
Contributor Author

@Neha Neha Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

removed the file from the PR

Neha
Neha commented Nov 5, 2024
View reviewed changes
Copy link
Contributor Author

@Neha Neha left a comment

Choose a reason for hiding this comment

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

all changes are done. Please re-review

components/MenuContext.tsx Outdated
{children}
</MenuContext.Provider>
const value = useMemo(
() => ({
Copy link
Contributor Author

@Neha Neha Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

removed the file from the PR

yarn.lock Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it is yarn.lock we do need this for fix version packages

@Neha Neha force-pushed the feat/implement-pre-commit-hooks branch from 8a1a9be to 0d8f1e9 Compare November 5, 2024 21:26
@Neha
Copy link
Contributor Author

Neha commented Nov 11, 2024

@giolaq as suggested, changes are done. Please recheck. Thanks

giolaq
giolaq reviewed Nov 11, 2024
View reviewed changes
Copy link
Contributor

@giolaq giolaq left a comment

Choose a reason for hiding this comment

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

part related to the pre-hooks and eslinting looks good.
Make sure you get the changes in main

components/CustomDrawerContent.tsx Outdated
@@ -19,7 +19,10 @@ export default function CustomDrawerContent(props: any) {

return (
<SpatialNavigationRoot isActive={isMenuOpen}>
<DrawerContentScrollView {...props} style={styles.container} scrollEnabled={false}>
<DrawerContentScrollView {...props} style={styles.container} scrollEnabled={false} contentContainerStyle={{
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is already merged, make sure you update to the latest main

@Neha Neha force-pushed the feat/implement-pre-commit-hooks branch from 2f1a44c to 8529ada Compare November 11, 2024 22:10
giolaq
giolaq approved these changes Nov 12, 2024
View reviewed changes
Copy link
Contributor

@giolaq giolaq left a comment

Choose a reason for hiding this comment

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

LGTM

@giolaq giolaq merged commit 615240e into AmazonAppDev:main Nov 12, 2024
@giolaq giolaq mentioned this pull request Nov 17, 2024
Closed
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giolaq giolaq giolaq approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement pre commit hooks
2 participants
@Neha @giolaq