-
Notifications
You must be signed in to change notification settings - Fork 506
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
feat: Modified format_checkers to add checker name to dictionary allow #1571
Conversation
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.
Looks good except for some changes. Also might as well add type hints.
@BreadGenie how do i add type hints? |
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.
This is looking promising and I'm approving the CI workflow to run, but it's got a few things
I've got a few things I'd want:
- Update to the docs to explain what format_checker does. I'm not sure we have really comprehensive format_checker documentation, so this would likely be a little note in the contributor docs where it explains the linters and stuff.
- A test of the new behaviour (I'd be willing to have the test come in a separate PR if you think it's going to take a lot of work setting up mock or refactoring so the file write happens in a separate function.)
@terriko Maybe I should make another PR for testing. I mean we could at least finalize this till I work on that? |
Here's python docs on typing and an intro to get an idea. If you're familiar with any typed programming languages you'll be able to pick it up fairly easily. |
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.
The linting tests are failing maybe because you've yet to install pre-commit. See our contributing guide for installation
@BreadGenie The linting test seems to work after I set up pre-commit, and I've removed the network request as requested. What are the next steps? |
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.
I'm re-approving CI to run. I'm basically just waiting on some documentation before this can be merged.
Alright I'll add some documentation for helper script! |
Codecov Report
@@ Coverage Diff @@
## main #1571 +/- ##
==========================================
- Coverage 81.60% 81.27% -0.34%
==========================================
Files 290 290
Lines 5790 5810 +20
Branches 953 957 +4
==========================================
- Hits 4725 4722 -3
- Misses 843 865 +22
- Partials 222 223 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
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.
Still looking for some documentation update to go with this before it can be merged.
fdb32d1
to
6442ad0
Compare
6442ad0
to
0caf48a
Compare
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.
Looks good now, thank you! This should be really handy going forwards.
fixes #1567