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

Tune writing style checker for plugin compatibility and custom apps #341

Merged
merged 4 commits into from
Apr 8, 2020

Conversation

nkuehn
Copy link
Collaborator

@nkuehn nkuehn commented Apr 3, 2020

  • provides a local "raw" vale command that the config can be directly passed to. This proved more robust when using the VSCode plugin.
  • adjust recommended VSCode workspace to use the config directly from the package to make transparent that it's not the same as an NPM version. Also allows for quicker local feedback in the editor when tuning the rules.
  • add a few acronyms that are common in software development
  • allow the "ZEIT" brand in acronymys (false negative, it's not an aconym).

@nkuehn nkuehn requested a review from emmenko April 3, 2020 15:42
@vercel
Copy link

vercel bot commented Apr 3, 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/110d5vt3z
✅ Preview: https://commercetools-docs-git-nk-writing-style-for-custom-apps-ab0e79.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.

Thanks! We should also probably update the readme.

@@ -14,7 +14,8 @@
"url": "https://github.com/commercetools/commercetools-docs-kit.git"
},
"bin": {
"commercetools-vale": "./bin/commercetools-vale.js"
"commercetools-vale": "./bin/commercetools-vale.js",
"vale": "./bin/vale-2.1.0"
Copy link
Member

Choose a reason for hiding this comment

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

We should be careful with this, as the symlinked binary name might conflict with other packages exposing such name.
We should try to use a more specific name.

Copy link
Member

Choose a reason for hiding this comment

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

Or we don't expose the npm binary and just have a "proxy" bash script or something that points to the specific binary version.
I suppose this is mostly the case when you want to use it in e.g. VSCode.
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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm renaming the symlink and adding it to the Readme. Forgot that you can install it globally and that would collide with a default vale installation.

Copy link
Member

Choose a reason for hiding this comment

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

It's not just the global installation, but also the node_modules/.bin symlinks

Copy link
Member

Choose a reason for hiding this comment

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

Have you considered my alternative proposal, of having a "proxy" script?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but the technical intent here is to get rid of any wrappers (for other reasons, too like keeping the color output).
is the new name ok?

Copy link
Member

Choose a reason for hiding this comment

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

My point was that if we just need access to the vale binary for the vscode setup, I wouldn't put it in the npm binary list because we already have the commercetools-vale binary.

In vscode you just point the binary to

"vscode-vale.path": "${workspaceFolder}/node_modules/@commercetools-docs/writing-styleles/vale-bin",
"vscode-vale.configPath": "${workspaceFolder}/node_modules/@commercetools-docs/writing-styleles/.vale.ini",

The vale-bin then is just a proxy script to the downloaded binary version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I got your point but I (as one of the actual users) do want to call it without going through a proxy script to get colored console output. Can you live with that fact? I wrote it in the above comment for a reason.

Copy link
Member

Choose a reason for hiding this comment

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

I think you're still missing my point. The commercetools-vale stays as-is, no changes there. I assume that users will primarily use that binary, if they want to.

Now, regarding the second binary: my question (which you still haven't answered) is whether this binary is only supposed to be used for the VSCode plugin or are there any other use cases for which you (as a user) would need to use it directly, instead of using the commercetools-vale binary.

No hard feelings, I'm just trying to understand the requirements as it's still not clear to me.

@vercel vercel bot temporarily deployed to Preview April 6, 2020 11:24 Inactive
@nkuehn nkuehn force-pushed the nk-writing-style-for-custom-apps-update branch from 0c70510 to 490a9ea Compare April 6, 2020 11:25
@vercel vercel bot temporarily deployed to Preview April 6, 2020 11:25 Inactive
@nkuehn nkuehn requested a review from emmenko April 6, 2020 11:26
@nkuehn
Copy link
Collaborator Author

nkuehn commented Apr 6, 2020

PS: the plugin now works for me, too :-)

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.

Let’s move on with this. Thanks again for looking into it.

@emmenko
Copy link
Member

emmenko commented Apr 8, 2020

Don't forget to assign the label, thanks

@emmenko emmenko force-pushed the nk-writing-style-for-custom-apps-update branch from 490a9ea to c7f4074 Compare April 8, 2020 12:16
@vercel vercel bot temporarily deployed to Preview April 8, 2020 12:16 Inactive
@nkuehn nkuehn added the 💅 Type: Enhancement Improves existing code label Apr 8, 2020
@nkuehn nkuehn merged commit fde7e76 into master Apr 8, 2020
@nkuehn nkuehn deleted the nk-writing-style-for-custom-apps-update branch April 8, 2020 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💅 Type: Enhancement Improves existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants