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
fix: installation breaks due to lefthook in postinstall #144
Conversation
Pull request title looks good 👍! If this pull request gets merged, it will cause a new release of the software. Example: If this project's latest release version is All merged pull requests will eventually get deployed. But some types of pull requests will trigger a deployment (such as features and bug fixes) while some pull requests will wait to get deployed until a later time. This project uses a special format for pull requests titles. Expand this section to learn more (expand by clicking the ᐅ symbol on the left side of this sentence)...This project uses a special format for pull requests titles. Don't worry, it's easy! This pull request title should be in this format:
If your pull request introduces breaking changes to the code, use this format:
where
Examples:
Need more examples? Want to learn more about this format? Check out the official docs. Note: If your pull request does multiple things such as adding a feature and makes changes to the CI server and fixes some bugs then you might want to consider splitting this pull request up into multiple smaller pull requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one questions, looks good overall 👍🏻
### [2.4.1](2.4.0...2.4.1) (2023-06-05) ### Bug Fixes * installation breaks due to lefthook in postinstall ([#144](#144)) ([e451443](e451443))
@@ -37,7 +37,7 @@ jobs: | |||
with: | |||
check-name: 'eslint results' | |||
only-pr-files: false | |||
fail-on-warning: true | |||
fail-on-warning: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this changed to false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's breaking the build, this also runs on Sample App
which has more than 60 warnings.
On a side note, why do we fail or warning? we don't on any other SDK? we only fail on errors. Why do we need to fail on warning as they at times very opinionated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This config change in this PR is a good change for the short-term. Especially because...
also runs on Sample App which has more than 60 warnings.
For a long-term solution, I prefer to make warnings be treated as errors and if there is a rule that the team feels is not needed, we ignore that rule.
I prefer this method because I don't see much value in lint warnings if they are not treated as errors. If lint warnings should not throw an error on the CI server and therefore be allowed to be merged into main
, why not instead add that lint rule to the lint ignore list? To me, those 2 things are the same thing: we either follow a lint rule, or we don't. Lint warnings, IMO, are a 🤷🏻 if we follow the rule or not.
No description provided.