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 --error-on-warn flag to exit 1 on warnings #345

Merged
merged 1 commit into from Oct 20, 2022

Conversation

mvargeson
Copy link
Contributor

@mvargeson mvargeson commented Sep 3, 2021

This addresses the scenario described in #314. I'm not sure if this is a desirable addition to the project but we are running into issues with automated dependency updates not taking into consideration patched dependencies and would like to prevent these warnings from making it onto our main branch.

The change is pretty minimal and follows the pattern that the --error-on-fail flag took. This changes the process.exit logic at the end of applyPatchesForApp and gives that function a new parameter, shouldExitWithWarning that looks at the flag boolean value. There are a couple of questions that I have:

  1. Does shouldExitWithWarning make sense? I may be misunderstanding terminology in variable names so please feel free to suggest something better.
  2. Do I need to consider environmental variables similar to --error-on-fail? I did not fully understand if that would be a good idea here.
  3. Would it be better to try to introduce a single flag that would take both warning and failures levels into account before deciding the exit code? Again, I mostly tried to just stick with the change that was the least amount of change but open to something else here.

Feel free to close out if not interested in this addition or request changes if this doesn't seem like the correct implementation.

@mvargeson mvargeson marked this pull request as ready for review September 3, 2021 05:49
@jraoult
Copy link

jraoult commented Sep 10, 2021

I would love to see it landing 👏. @ds300 any chance you would consider this?

@jraoult
Copy link

jraoult commented Oct 25, 2021

@mvargeson have you published your fork on NPM by any chance? I really want this and I'm don't think it will be merged upstream anytime soon (the last activity I can see on this repo was in March).

@mvargeson
Copy link
Contributor Author

mvargeson commented Oct 25, 2021

@jraoult I haven't but feel free to take the branch from my fork and do so. If you prefer a more simple way to use, it may be easiest to create branch on your own fork, apply the git patch I linked below, and then make use of that directly in your package.json dependencies.

Alternatively, if you only want the min change required then here is the branch: mvargeson:mvargeson/issue-314/error-on-warn-min-change.

Here is the git patch: 0001-Exit-on-warning-and-errors.patch.tar.gz.

Here is the patch to the installed node_modules dist file although you can't really use patch-package on itself so probably no terribly useful: patch-package+6.4.7.patch.tar.gz.

@jraoult
Copy link

jraoult commented Oct 26, 2021

@mvargeson thanks for your reply.

The reason I'm asking about publishing to NPM is that this project needs a build step since it is written in TS and my good old legacy yarn is not able to get that build at install time properly, so the git dependency style won't work for me.

I'll publish a fork under my namespace if you don't mind then, it's easier.

@mvargeson
Copy link
Contributor Author

mvargeson commented Jul 21, 2022

There have been no updates to this repository in over a year; closing this out. Feel free to reopen if the project is revived at any point.

@mvargeson mvargeson closed this Jul 21, 2022
@mvargeson mvargeson reopened this Oct 20, 2022
@orta
Copy link
Collaborator

orta commented Oct 20, 2022

I think this PR as-is is reasonable, I can see the idea of switching to a --fail-on=warn or --fail-on=error but I don't think it's compelling enough to warrant breaking existing flows, doing server bumps etc. So, lets have this in 👍🏻

@orta orta merged commit 86ed9f6 into ds300:master Oct 20, 2022
@ds300
Copy link
Owner

ds300 commented Oct 25, 2022

This was just released in v6.5.0! Thanks for your contribution 🙏🏼 🎉

@jraoult
Copy link

jraoult commented Oct 25, 2022

Oh my 🎉 I'm sadly not using patch-package anymore, but my 2021 self is thanking you. It probably means the original issue can be closed as solved as well @orta (cf. #314)

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.

None yet

4 participants