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

Do non-builtin styles work? #4

Closed
ChrisChinchilla opened this issue Nov 26, 2019 · 12 comments
Closed

Do non-builtin styles work? #4

ChrisChinchilla opened this issue Nov 26, 2019 · 12 comments
Labels
question Further information is requested

Comments

@ChrisChinchilla
Copy link
Contributor

I was playing with adding the action to a repository, and while any of the inbuilt vale styles worked, nothing else did. So when I looked at the example you have here #1 I'm not actually sure that the non vale styles there work either.

For example:

This will likely be fixed by toolkit/issues/186.

Should flag two write-good warnings, but there are none, even though , as far as I can tell write-good checks should be running.

@jdkato
Copy link
Member

jdkato commented Nov 26, 2019

Yes, my intent is for all of Vale's features to work. You can see here that write-good is indeed working.

The linked PR doesn't reference write-good (commit-wise, it has fallen behind but my intent was to simply demonstrate the annotations).

@ChrisChinchilla
Copy link
Contributor Author

Hmm, well I am utterly confused then, been playing with this for ages and getting nowhere…

This is a test PR - https://github.com/kauri-io/Content/pull/109/files
Action config - https://github.com/kauri-io/Content/blob/master/.github/workflows/language-lint.yml

It's a bit more complex than your example, but as far as I can tell, it's correct, even with an empty folder to checkout remote styles (not sure that's needed). But as you can see all I get in that PR is vale errors and nothing else.

Any insights would be appreciated as I feel like there's something really obvious I'm missing… Then I'll write some blog posts and docs 😁

@jdkato
Copy link
Member

jdkato commented Nov 30, 2019

I can't say that I completely understand why you're getting output from Vale.Spelling, but here are my suggestions:

  1. Some potentially relevant error messages from Vale were being unintentionally suppressed in the Action output. I've fixed this in v1.0.2, so I would suggest bumping the action version here.

  2. As is, your repository structure won't work as intended: You have a vocab.txt file under .github/styles, but your StylesPath is consensys-styles (from the remote .vale.ini file). I would suggest creating a .vale.ini file at the root of the kauri-io/Content repository with the correct StylesPath:

    # NOTE: This file inherits from a remote configuration.
    #
    # See `.github/workflows/main.yml`.
    StylesPath = .github/styles

    This will override the value in the remote config file, while still inheriting the other settings.

If these changes don't help, I'll have more time to look into this next week.

@ChrisChinchilla
Copy link
Contributor Author

Ha! Not sure if it was removing the styles path that wasn't even needed and I hadn't noticed was there, or upgrading the action, but it works now!

https://github.com/kauri-io/Content/pull/110/files

It still keeps checking files that weren't changed, but I think that's more of an action configuration issue than a an issue with this specific action.

Sometimes you just need a second pair of eyes…

@ChrisChinchilla
Copy link
Contributor Author

Though actually, if you look here - https://github.com/kauri-io/Content/pull/111/files

Things are working, but too much is working, it's running tests that are specifically set to NO in https://raw.githubusercontent.com/ConsenSys/Style-Guide/master/.vale.ini

@jdkato
Copy link
Member

jdkato commented Dec 6, 2019

I've finally had a chance to look into this and, frankly, I'm still not exactly sure why you're seeing these results (Actions are somewhat difficult to debug and test).

However, I did find and fix a potential race condition when there's no local configuration file (now v1.0.3): sometimes Vale could try to install external styles prior to downloading the external configuration file.

I set up two test branches that emulate your repository's structure (two external styles, no local config file) and it's working as expected.

@ChrisChinchilla
Copy link
Contributor Author

@jdkato Still the same problem I'm afraid, with disabled default vale checks running even though they're disabled.

Here's a couple of thoughts.

The action uses Vale 2 beta, none of the styles I'm using have been tested with version 2, could that be an issue?

Your test branches are using version 2, mine 2-beta.2, and yours output JSON style debug messages, and my output doesn't. i.e.,

Installing style 'write-good' ...
vale --no-exit --output=JSON --mode-rev-compat --config=/github/workspace/tmp-1ghEVve57palh.ini install write-good https://github.com/errata-ai/write-good/releases/latest/download/write-good.zip
{
  "Msg": "Successfully installed '/github/workspace/styles/write-good'",
  "Success": true,
  "Error": ""
}

mine:

Installing style 'write-good' ...
/bin/vale --no-exit --output=JSON --mode-rev-compat --config=/github/workspace/tmp-17MFMtkdkskC6.ini install write-good https://github.com/errata-ai/write-good/releases/latest/download/write-good.zip

Which is odd… But as you say actions are damned hard to debug… :(

@ChrisChinchilla
Copy link
Contributor Author

Interestingly if you look on this check (the lint stage) (https://github.com/kauri-io/Content/pull/116/checks?check_run_id=348595089) you also see that using my config and downloaded styles, no errors are generated, even though they should be, then in the vale step it just defaults to using the internal checks, and checks everything, not the changed files.

Gah, actions have so much promise, but are too much of a black box 😩

@mattbrailsford
Copy link

I've just spent the last hour or so trying to get custom styles to work and I can't for the life of me get it to work either.

My repo is here https://github.com/vendrhub/vendr-documentation using a fairly basic setup yet I have a rule here https://github.com/vendrhub/vendr-documentation/blob/master/.github/styles/Vendr/Terms.yml which should suggest "back-office" should be "backoffice", but this page is full of "back-office" https://github.com/vendrhub/vendr-documentation/blob/45ccd055909f8a5dc9e4ad83ade838687792ed3f/content/core/1.0.0/getting-started/installation.md and Vale is not showing any warnings about this https://github.com/vendrhub/vendr-documentation/runs/480670597

@jdkato
Copy link
Member

jdkato commented Mar 2, 2020

@mattbrailsford You need to include your style in your config file:

https://github.com/vendrhub/vendr-documentation/blob/a6538461e7aeb290ea95486261eea1a7b9a2c666/_vale.ini#L5

You currently only have BasedOnStyles = Vale, but you want BasedOnStyles = Vale, Vendr.

@mattbrailsford
Copy link

Ahhhh. Thanks, I’ll give that a try 👍🏻

Would be great if something like this could be added as an example as I was really struggling to find examples of setting up local styles properly.

@jdkato jdkato added the question Further information is requested label Mar 4, 2020
@ChrisChinchilla
Copy link
Contributor Author

Ahhhh. Thanks, I’ll give that a try 👍🏻

Would be great if something like this could be added as an example as I was really struggling to find examples of setting up local styles properly.

This is something I definitely have in progress, will finish it up asap.

@jdkato jdkato closed this as completed Jun 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants