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

Jump to bottom

pre-commit: Add codespell and other checks #263

Merged
merged 1 commit into from May 6, 2022
Merged

pre-commit: Add codespell and other checks #263

merged 1 commit into from May 6, 2022

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented May 6, 2022 •
edited

@jezdez @mattkram As discussed at #201 (comment)

@verhulstm
Copy link
Collaborator

@verhulstm verhulstm commented May 6, 2022

what do you mean by tests here? i don't see anything i understand to be test?

@cclauss cclauss changed the title pre-commit: Add more tests pre-commit: Add codespell and other checks May 6, 2022
@cclauss cclauss force-pushed the more-pre-commit branch 2 times, most recently from e7441c0 to 6262d6c Compare May 6, 2022
@cclauss cclauss mentioned this pull request May 6, 2022
Open
11 tasks
fpliger
fpliger approved these changes May 6, 2022
View changes
Copy link
Collaborator

@fpliger fpliger left a comment

that's great! thank you @cclauss

There are some conflicts with main and I'd like @jezdez or @mattkram to check it out since it could have overlap with other things happening in parallel PRs but lgtm!

mattkram
mattkram requested changes May 6, 2022
View changes
Copy link
Contributor

@mattkram mattkram left a comment

Thank you for this second commit. I have recently merged #245, which implements a few of these already. Would you mind please merging those changes in and resolving conflicts in .pre-commit-config.yaml?

It may make sense to revert all changes and re-apply pre-config to ensure we are not introducing any unnecessary changes due to the merge.

Thanks!

.pre-commit-config.yaml Outdated Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@cclauss
Copy link
Contributor Author

@cclauss cclauss commented May 6, 2022

Re-re-re-based.

mattkram
mattkram approved these changes May 6, 2022
View changes
Copy link
Contributor

@mattkram mattkram left a comment

Looks great. Thanks @cclauss !

@mattkram mattkram merged commit fadb4a6 into pyscript:main May 6, 2022
3 checks passed
@cclauss cclauss deleted the more-pre-commit branch May 6, 2022
@cclauss
Copy link
Contributor Author

@cclauss cclauss commented May 6, 2022 •
edited

Thanks for merging! Flake8's ability to find undefined names is a superpower for finding bugs.

I am a bit worried about constantly having to add entries to the builtins on line 5 of setup.cfg:

builtins=Element,PyItemTemplate,PyListTemplate,pyscript

I have experience on other projects that these names that appear from nowhere confuse both humans and linters, type checkers, etc. I know there will be more of these and it is difficult to put the genie back in the bottle...

Would it be possible to adopt a more Pythonic syntax like:

from pyscript import Element, PyItemTemplate, PyListTemplate

to reduce confusion for both developers and their tools? These classes, functions, and variables will only be available in pyscript environments so it would be a good idea to link them back to pyscript.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fpliger fpliger

@mattkram mattkram

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@cclauss @verhulstm @fpliger @mattkram