-
Notifications
You must be signed in to change notification settings - Fork 206
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: run eslint from PATH #654
Conversation
Hi! I'd like some feedback on this, please.
|
@JustinBeckwith would you be a good person from whom to request code review on this, please? Tagging you because you mentioned PRs being welcome on the issue (#490) that I'm trying to solve. |
@richardbarrell-calvium this change seems reasonable to me, but I would like to test in a few environments before landing. Could I bother you to rebase and push, for whatever reason it seems like actions did not kick off. |
Alternatively, @richardbarrell-calvium, if you could check the box that grants contributor write access to your branch, we can drive this to completion. Thank you! |
Rationale: - npm and Yarn both put (a directory containing) the eslint binary on PATH during "npm run foo", so take advantage of that to invoke eslint in a way that is compatible with Yarn workspaces. - (Node that `node_modules/eslint/...` may not exist, because the module may have been installed in another `node_modules` further up the filesystem tree. Yarn does ensure that the binary gets symlinked into `node_modules/.bin`.) - This should fix issue google#490.
cf0a311
to
fb41801
Compare
Hi there! @bcoe apologies for not replying sooner, the email notification got buried in my inbox. My bad. @alexander-fenster I've rebased my branch on main and pushed it. I'm afraid I have no idea where that checkbox is. I can grant you write access to https://github.com/calvium/gts from the "Collaborators and teams" page? GitHub's UI is telling me that the CI actions weren't invoked because workflow approval is required. Here is a screenshot of what I'm seeing: |
@alexander-fenster It would be great to see this move forward. If you're worried about compatibility, you could append |
@danielbankhead could you please have a look at this one? |
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.
LGTM
@richardbarrell-calvium please pull from the base branch and I can get this merged. |
@danielbankhead Thanks! I've merged |
Looking into the Windows test timeout... |
@richardbarrell-calvium Nice! Looks like it passed. Do you mind pulling from |
I've merged main and pushed again. |
Rationale:
node_modules/eslint/...
may not exist, because the module may have been installed in anothernode_modules
further up the filesystem tree. Yarn does ensure that the binary gets symlinked intonode_modules/.bin
.)