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

PR: Second round: clean imports and qualifiers #584

Merged
merged 17 commits into from Dec 10, 2022
Merged

PR: Second round: clean imports and qualifiers #584

merged 17 commits into from Dec 10, 2022

Conversation

edreamleo
Copy link
Contributor

@edreamleo edreamleo commented Dec 10, 2022

This PR continues PR #583. This PR suggests a consistent style for imports, thereby regularizing (minimizing) qualified names. All changes should be straightforward and uncontroversial.

This PR adds or maintains (with new comments) fully qualified names for builtins, change, fscommands, and wildcards when those names would clash with other names.

This PR removes qualifications for modules (like pyobjects and several others) whose names conflict neither with the names other modules (in stdlib or Rope) nor other vars.

Note: The qualification of ast remains completely unchanged. Some files use rope.base.ast; other files access rope.base.ast with an unqualified ast.

@edreamleo edreamleo marked this pull request as draft December 10, 2022 12:03
@edreamleo edreamleo changed the title PR: Second round of cleaning imports and qualifiers PR: Second round: clean imports and qualifiers Dec 10, 2022
@edreamleo edreamleo marked this pull request as ready for review December 10, 2022 14:01
@edreamleo edreamleo marked this pull request as draft December 10, 2022 14:02
@edreamleo edreamleo marked this pull request as ready for review December 10, 2022 14:21
@edreamleo edreamleo marked this pull request as draft December 10, 2022 14:24
@edreamleo edreamleo marked this pull request as ready for review December 10, 2022 14:32
@edreamleo
Copy link
Contributor Author

@lieryan I plan no more commits to this branch.

@@ -1,7 +1,6 @@
import rope.base.exceptions
import rope.base.pyobjects
# import rope.base.exceptions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import?

Suggested change
# import rope.base.exceptions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. I'll remove that comment.

@lieryan
Copy link
Member

lieryan commented Dec 10, 2022

@edreamleo thanks for doing this.

Maybe rather than adding comments about the full import qualification, we should just document this centrally?

I'd recommend a new section in docs/contributing.rst describing the rationale for the seemingly "consistently inconsistent" import style. It should say that we defaults to one level import style, and it should document the list of modules that should use full import qualifications. Then can we remove the inline comments. All this can be done in a separate PR.

@edreamleo
Copy link
Contributor Author

@lieryan But who reads "central" documentation? I like to document the code in place if it's not too odious.

@edreamleo
Copy link
Contributor Author

@lieryan I see you have already updated master, so I'll use your suggested code.

@edreamleo
Copy link
Contributor Author

@lieryan I'm good with this PR now.

We could quibble about exactly when to use full qualification, but who would want to do that :-)

@lieryan
Copy link
Member

lieryan commented Dec 10, 2022

Import style consistency is nice, but I'm personally not too fussed if sometimes we forget about it and a couple non compliant imports slipped in 🤷‍♂️.

If necessary, in the future, we can write a pre-commit/GHA script to enforce the import style. So that it'll always be consistent without needing the comments.

It seems like it can be a useful capability for findsimilar/restructuring as well to be able to create search patterns that matches imports based on their style. But we're all ast experts here 😉, seems like a simple checker can even be just a simple standalone script.

Though I think I'm getting too much into ideas mode here, let's pull back before I get in too deep into this.

@lieryan
Copy link
Member

lieryan commented Dec 10, 2022

We could quibble about exactly when to use full qualification, but who would want to do that :-)

Agreed, the comments are fine as is for now, it's not a blocker for this PR.

@lieryan lieryan merged commit ae7f56d into python-rope:master Dec 10, 2022
@edreamleo edreamleo deleted the ekr-clean2 branch December 10, 2022 15:51
@lieryan lieryan added this to the 1.6.0 milestone Dec 14, 2022
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.

None yet

2 participants