-
Notifications
You must be signed in to change notification settings - Fork 30
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: clean up & refactor majorly #171
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
everything seems to work post python version 3.11, I'll shift make name change, ie src->tesk at least, rest need to be discussed. |
and addd help command for the repo to explain the scripts available
Also, why are Python 3.8, 3.9 and 3.10 failing, @JaeAeich? On the other hand, it's not clear to me why we are even testing multiple Python versions, TESK is not a library. We pick one version and use that version, no need to support multiple Python versions... |
I have no idea, lack of backwards compatibility ig, I haven't touched any code of tesk_core yet. Should I freeze python version to 3.11 and above (for pyproject.toml)? Will remove the other versions from API. Also core seems to use kubernetes client v9 and rn its on v29 😶🌫️ , I'll definetely have to refactor core code as well in order to start to use same k8s client version on api. |
They're here: https://github.com/elixir-cloud-aai/TESK/settings/branches You need to edit the rule for branch Anyway, I have done that now in case you don't have access to that setting. |
Yeah I don't have the rights, but thanks for updating it :) |
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.
Amazing job @JaeAeich. Of course the PR is waaay too big to review in depth (and also it's not always clear what was new code and what was just renamed but not picked up by the system, e.g., TESK-core code and corresponding tests). But I understand that this was +/- necessary to keep tests passing (which is commendable). Still, in the future, I hope we will get small, iterative PRs ;-)
Anwyay, I suggest that after addressing (or acknowledging and potentially creating issues for the future) the comments and a final review, we should be good to go for merging. There are likely still several issues (mostly legacy ones), but we can address them iteratively.
We need Apart from that since this PR has grown big enough why not discuss some extra things I found while going through other repos and etc etc.
|
Sure, feel free to add (low priority) issues for these :) |
|
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.
We're getting there, just a few small issues before we can merge.
However, sth is weird about your Git workflow. Several (but not all) files are shown here as completely new, even though there were just moved or a few lines were changed (e.g., pyproject.toml
, some but not all test files moved from service
to test_service
). Try to figure out why that is and avoid it in the future, because it makes things hell to review and also you git blame
will show you as the author of all of that code, even if you are not.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
I don't think there is anything wrong with my workflow, the issue was that the code files were not formatted at all and had inconsistent spacing even between two words 😅 , plus fixing some minor issues to comply with Google's python coding guide like changing string concatenation to f string, these kind of small fixes were cluttered all around the code base. I believe that is the cause of this weird blame thing. But if it happens again I'll be sure to check into it :) |
@uniqueg please check if the badge I added is correct. |
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.
LGTM, I'm gonna merge. From now on please keep the PRs small and semantic 😆
Description
commit-lint
andmerge
conflicts)24mb
,150
->174
)fixes - #152 #153 #154 #155