Skip to content

WIP: Add support for quiet level #22

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

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

GuillaumeFavelier
Copy link

On mne-python, we would like to have access to the --quiet-level parameter to suppress some messages.

This PR adds support for the quiet level.

It's still work in progress, I am not familiar with actions design and I need help to write useful tests.

@GuillaumeFavelier
Copy link
Author

The failures seem similar to those on master (f534087)

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

@peternewman can you look?

@peternewman
Copy link
Collaborator

Hi @GuillaumeFavelier ,

The failure running against git master within the test was because we had some breaking changes which were released in Codespell 2.0. The other bats failures are seemingly something that slipped through the net with how we're simulating part of our Docker run, I think I'm getting there now.

You're certainly welcome to add this option to the action, there is also another option now Codespell 2.0 has been released, you can just put everything in a config file ( https://github.com/codespell-project/codespell/#using-a-config-file ), which might be a lot easier, it depends on your preferences. Although I appreciate you might want it quiet for your users and verbose for your CI or vice versa.

@peternewman peternewman marked this pull request as draft December 2, 2020 19:38
@peternewman
Copy link
Collaborator

I've put this into draft as well for now @GuillaumeFavelier feel free to switch it back as appropriate, let me know if you need a hand with the BATS tests (once I've wrangled them into behaving again!).

Copy link
Collaborator

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

LGTM aside from the tests

@peternewman
Copy link
Collaborator

@GuillaumeFavelier if you update then the existing tests should come good and you can see some working examples to copy/paste in test/test.bats to add a test for this.

Copy link
Collaborator

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

LGTM @GuillaumeFavelier . Would you mind adding a test for this, then we can merge it?

@GuillaumeFavelier
Copy link
Author

I tried 8687724 but I have to admit, I still do not grasp exactly how it works

@GuillaumeFavelier
Copy link
Author

Sorry for the huge delay 🙏
I updated the branch and tried 19753a4 to "disable the bats diagnostics":
Testing / Diagnose bats are green but not Testing / Run tests though

@peternewman
Copy link
Collaborator

Sorry for the huge delay 🙏

No worries @GuillaumeFavelier

I updated the branch and tried 19753a4 to "disable the bats diagnostics":

Sorry, I explained that really badly/not at all. I actually meant remove that line. Unfortunately #24 doesn't make it as simple as I'd hoped.

Testing / Diagnose bats are green but not Testing / Run tests though

I've now changed diagnose bats to mimic your test run (one of the things I hoped to be able to automate but it hasn't quite happened yet).

So the dummy bats diagnostics run, shows what bats will be trying to check:
https://github.com/codespell-project/actions-codespell/pull/22/checks?check_run_id=1889049192#step:5:28

Which as you can see returns 0 errors:
https://github.com/codespell-project/actions-codespell/pull/22/checks?check_run_id=1889049192#step:5:40

But in the bats test you're expecting errorCount=$QUIET_LEVEL_MISSPELLING_COUNT with QUIET_LEVEL_MISSPELLING_COUNT=1
https://github.com/codespell-project/actions-codespell/pull/22/files#diff-e98dd325e580e64931383363be3819d3e40938c4ad0fb8a868056d4c58aa80f8R88

So as per the previous comment you'll need to make those two match up for starters.

@GuillaumeFavelier
Copy link
Author

So as per the previous comment you'll need to make those two match up for starters.

Oh! Then what I want to test is not the number of errors but the number of warnings. Is there a way to do that? Or the opposite let's say turn warnings into errors?

@peternewman
Copy link
Collaborator

So as per the previous comment you'll need to make those two match up for starters.

Oh! Then what I want to test is not the number of errors but the number of warnings. Is there a way to do that?

Probably not based on the count, but you could also do line matching as some of the other examples show.

If you had a warning sandwich:

ERROR
WARNING
ERROR

Then you could check the line contents of all three, and then disable the warning and check the two still matched maybe.

In theory you could just count all the lines returned, but as you'll see with the add/remove matcher lines that can get a bit complicated if we can't fake them up correctly.

Or the opposite let's say turn warnings into errors?

I'm not sure off-hand if there's a way to do that in Codespell currently, although maybe we should generally add one.

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.

3 participants