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

Writing Style Linter: Update vale and prevent binary from being published #366

Merged
merged 8 commits into from
Apr 16, 2020

Conversation

nkuehn
Copy link
Collaborator

@nkuehn nkuehn commented Apr 15, 2020

This fixes the notorious issue that the wrong vale binary was executed on linux hosts, e.g. CI. I updated .gitignore when changing the binary naming convention but oversaw .npmignore.

Since this requires a new package version anyways, I am updating the vale version to a recent bugfix in the same step.

@nkuehn nkuehn added 🐛 Type: Bug Something isn't working 🤖 Type: Dependencies Dependency updates or something similar labels Apr 15, 2020
@nkuehn nkuehn requested a review from emmenko April 15, 2020 20:24
@vercel
Copy link

vercel bot commented Apr 15, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/commercetools/commercetools-docs-kit/q40pzveya
✅ Preview: https://commercetools-docs-kit-git-nk-fix-and-update-vale-binary.commercetools.now.sh

Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

You also need to update the version for the bin

@emmenko
Copy link
Member

emmenko commented Apr 15, 2020

That's also what I meant here: #341 (comment)

With that also, we can read the vale version from the package.json and use it in the proxy script, instead of having the version hardcoded multiple times.

To avoid this situations where we forget to update both versions.

@vercel vercel bot temporarily deployed to Preview April 15, 2020 20:32 Inactive
@nkuehn
Copy link
Collaborator Author

nkuehn commented Apr 15, 2020

That's also what I meant here: #341 (comment)

With that also, we can read the vale version from the package.json and use it in the proxy script, instead of having the version hardcoded multiple times.

To avoid this situations where we forget to update both versions.

OK, you beat me to try it now. I've just pushed a wrapper that is actually preserving all console interaction and coloring etc by not using the fat shelljs wrapper but calling a process spawn that directly connects the stdin / stdout / stderr pipes without node.js being able to read them.

That did the job, so here we go.

We could remove shelljs in the other script, too - I didn't do it here because I don't remember why it was introduced from the beginning.

@nkuehn nkuehn requested a review from emmenko April 15, 2020 20:51
@vercel vercel bot temporarily deployed to Preview April 15, 2020 20:52 Inactive
Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

Thanks 💯

@vercel vercel bot temporarily deployed to Preview April 16, 2020 07:37 Inactive
@emmenko
Copy link
Member

emmenko commented Apr 16, 2020

@nkuehn I pushed a commit to reuse the common logic between the bin scripts. Please have a look

Comment on lines -15 to -21
const configParamPosition = params.findIndex((p) => p === '--config');
if (
params[configParamPosition + 1] &&
params[configParamPosition + 1].startsWith('-')
) {
params = params.filter((p) => p !== '--config');
}
Copy link
Member

Choose a reason for hiding this comment

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

As far as I understand, this is not necessary anymore, as for VSCode we can use the "config" option of the plugin and use the vale-bin binary.

@vercel vercel bot temporarily deployed to Preview April 16, 2020 19:18 Inactive
@nkuehn nkuehn force-pushed the nk-fix-and-update-vale-binary branch from 4600a73 to 3c7a971 Compare April 16, 2020 19:23
@vercel vercel bot temporarily deployed to Preview April 16, 2020 19:23 Inactive
@nkuehn
Copy link
Collaborator Author

nkuehn commented Apr 16, 2020

@emmenko since you moved the binary to the root folder the ignore rule was a bit dangerous to also cover the vale-styles directory so I renamed it to just styles which is the convention anyways in vale.

I overhauled the README to explain the VSCode usage and some other things.

@nkuehn nkuehn merged commit 8e6ac90 into master Apr 16, 2020
@nkuehn nkuehn deleted the nk-fix-and-update-vale-binary branch April 16, 2020 19:37
@emmenko
Copy link
Member

emmenko commented Apr 16, 2020

Perfect, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Type: Bug Something isn't working 🤖 Type: Dependencies Dependency updates or something similar
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants