-
Notifications
You must be signed in to change notification settings - Fork 27
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
Test conversion for all plugins #52
Conversation
Codecov Report
@@ Coverage Diff @@
## main #52 +/- ##
==========================================
- Coverage 80.99% 80.47% -0.53%
==========================================
Files 23 23
Lines 1142 1183 +41
==========================================
+ Hits 925 952 +27
- Misses 217 231 +14
Continue to review full report at Codecov.
|
|
||
# how do we deal with keywords ? | ||
# do we try to validate ? Or do we just | ||
# assume users won't try to create a command named | ||
# `npe2_tester.False.if.for.in` ? | ||
_dotted_name = f"(({_identifier}\\.)*{_identifier})" | ||
_dotted_name = f"(({_identifier_plus_dash}\\.)*{_identifier_plus_dash})" |
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 did this in a conflicting way in #51. the way you did it here looks better.
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.
very nice.
I approved, but I wish there a way to surface these checks separately from the required CI checks.
If you mean you'd like these not to run all the time on CI... absolutely. I had temporarily set the trigger to be "on push" so I could test it, but just made it to "workflow_dispatch" so it will only run on demand. |
need some stuff here, so merging. This workflow isn't "active" yet. just on manual trigger. |
This is an experiment to see if we can determine how successful the conversion tool will be across all known plugins.