-
Notifications
You must be signed in to change notification settings - Fork 5
refactor: update linter tools, python dependency and add types to app… #165
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
Conversation
ok let me fix the issue |
Thanks for the (much needed) update 💪🏻 |
sure. Thank you for your prompt review @woile 🙇 |
Should we add 3.12 and 3.13 job? |
I'm using python 3.13 and the tests passed on my machine. I think we're good to add python 3.12 and 3.13. |
Let me add black back |
probably ruff needs to be updated and black removed: For the ruff check decli/ tests/
ruff format --check decli/ tests/ examples/ and for the
|
Ok let me fix it. Thanks for the suggested changes |
I added |
Please don't add |
Ok let me revert the changes |
a8765d8
to
3d37e7d
Compare
Fixed. Hope the CI passes next time. |
|
The CI should pass this time. |
Is it possible that the issue is happening because the dev dependencies were split into different groups? |
Ok I see the problem. Let me fix it in the next iteration. |
Yes, I didn't notice that I added a new dependency group. |
I forgot to generate new poetry.lock |
Is that because my poetry is too new? I'm using poetry 2.1.3 |
try doing |
scripts/test
Outdated
pr ruff decli/ tests/ | ||
pr mypy decli/ tests/ | ||
ruff check | ||
mypy |
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.
they are missing the pr in the front
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.
Fixed.
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 curious about the reason pr
is needed for running the scripts. Does CI depend on pr
?
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 action is creating a virtual env, and if you run the modules through poetry, poetry by default will detect the current virtualenv. Otherwise you'd have to "activate" the virtualenv in the action. Why the action is using virtualenv? probably because of issues with github action in the past, that may or may not be solved now
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.
Thanks for your detailed explanation :)
|
Probably need to renew the token?
|
https://github.com/codecov/codecov-action/tree/3.x?tab=readme-ov-file#usage I'm not entirely sure if token is needed since this repo is public. Edit: Yes, it's required. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #165 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 102 102
=========================================
Hits 102 102
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…lication