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

refactor(import): Remove meaningless underscore imports #2375

Conversation

MortalHappiness
Copy link
Member

@MortalHappiness MortalHappiness commented Apr 23, 2024

Tracking issue

N/A

Why are the changes needed?

There are many import pkg as _pkg patterns in the codebase, and many of them are meaningless. For example, there are some files containing the following weird code snippet

import os
import os as _os

import os
import os as _os

which imports the os package twice.

Based on my own inference, perhaps the predecessors wanted to prevent exporting package names. However, renaming the builtin packages is meaningless.

What changes were proposed in this pull request?

This PR only deals with the import pkg as _pkg case. There are still many cases but refactoring them may introduce some unexpected behaviors. One can find these cases by searching as _ string in the codebase.

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

@wild-endeavor wild-endeavor merged commit 86f5661 into flyteorg:master Apr 23, 2024
47 checks passed
@eapolinario
Copy link
Collaborator

How did you choose which ones to remove? Asking because we still have a fair amount of imports like that: https://gist.github.com/eapolinario/c340dd6fae56c25729091f616b6b1cdd.

@MortalHappiness
Copy link
Member Author

@eapolinario I choose the imports with import pkg as _pkg pattern. I do not remove those imports like import nested.pkg as _pkg or from pkg import obj as _obj.

I do not remove those imports because I do not know the context why the predecessors rename the imports by adding the underscores as I mentioned in the PR description. Perhaps they want to keep them private? I am afraid that my changes will break some functionalities, so only those obviously meaningless imports are deleted.

fiedlerNr9 pushed a commit that referenced this pull request Jul 25, 2024
tyty
Signed-off-by: Chi-Sheng Liu <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants