-
Notifications
You must be signed in to change notification settings - Fork 64
refactor: improve code readability with more descriptive naming (#133) #141
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
Signed-off-by: Hijanhv <[email protected]>
Signed-off-by: Hijanhv <[email protected]>
Signed-off-by: Hijanhv <[email protected]>
Signed-off-by: Hijanhv <[email protected]>
564a4dc to
6c14a27
Compare
|
refactor: improve code readability with more descriptive naming (#133)
|
|
hi thanks for PR, please run prettier formatting and python black for linting so there are no effects in files which have no changes |
|
|
|
Oh I like that PR :-) |
|
Please ping me if its good to merge @LogicalGuy77 |
thanks , but there seems to be issue .Hey, I ran the frontend tests (npm run test:prod -- --browsers=ChromeHeadless --watch=false) and they all failed with TypeError: Cannot redefine property: location Looks like the test setup is trying to mock or redefine window.location, which isnβt allowed in ChromeHeadless. |
frontend/src/types/kubeflow.d.ts
Outdated
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 don't think we need a type file for kubeflow.
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.
same reason, common lib already has kubeflow types defined
|
Hey, Glanced over your PR. Also, Iβd suggest making changes in smaller batches rather than pushing so many updates at once, large diffs like this can easily break parts of the app and make tests fail. Keep up the good work! |
Hello and thank you for the review. Regarding one point I disagree strongly. Also internal variable should be long pronounceable and expressive. "cfg" is wrong, "configuration" is right. Long pronounceable and expressive variable names and being able to write code as English text separates junior from senior engineers in hiring interviews. There are endless sources about the reasoning behind this. But it is definitively true. So the idea from the first post is in general right:
|
|
@juliusvonkohout makes sense, I meant that tsconfig file should not be effected by these changes, besides that this looks great, I will run this locally and test it out., should be good to go then |
|
What changes shall I do now @LogicalGuy77 @juliusvonkohout |
we can first merge what works and is clarified and then you can raise a follow up PR for the remaining changes. |
|
hi i removed the others changes and kept only these |
|
Thank you, this looks a lot cleaner. Can you sign off your commit? |
π Overview
This pull request addresses Issue #133 β "Improve Code Readability with More Descriptive Naming."
Shortened or unclear identifiers have been replaced with more descriptive and expressive ones across the backend and frontend codebases.
This enhances maintainability, self-documentation, and readability for all contributors.
π§© Backend Changes
cfgβconfigurationsvcβinference_servicegvkβgroup_version_kindFILE_ABS_PATHβABSOLUTE_FILE_PATHinference_service_gvk()βinference_service_group_version_kind()_get_k8s_object()β_get_kubernetes_object()K8S_*βKUBERNETES_*π» Frontend Changes
currNamespaceβcurrentNamespacepollSubβpollingSubscriptionnsSubβnamespaceSubscriptionsvcβinferenceServicetypes/kubeflow.d.tsfor shared type definitions.configβdialogConfigresβdialogResponsename/valuepairs in chip components.tsconfig.app.jsonpaths for type safety.βοΈ GitHub Workflow Updates
.github/workflows/*.yamlfiles.π Benefits
β Checklist
π Linked Issue
Closes #133