Skip to content

Normalize import statements and make tests more robust to vendoring#399

Merged
aiuto merged 8 commits intomainfrom
imp
Aug 18, 2021
Merged

Normalize import statements and make tests more robust to vendoring#399
aiuto merged 8 commits intomainfrom
imp

Conversation

@aiuto
Copy link
Copy Markdown
Collaborator

@aiuto aiuto commented Aug 4, 2021

These are non-functional changes to aid users who import the library to their own repo.

The correct way to import is to list the package, relative to the repo: E.g. from private import helpers

  • none should use 'import rules_pkg.*', that breaks if you rename the repo
  • raw imports are harder to find and automatically transform

And fix some tests so they can be imported into other places.

Tested by importing the repot into Google. Not every test builds and runs, but we are getting closer.

@aiuto
Normalize import statements.
7eed68f 
- none should use 'import rules_pkg.*', that breaks if you rename the repo
- import from the top level.

The correct way to import is `from private import helpers`

And fix some tests so they can be imported into other places.
@aiuto aiuto requested review from dannysullivan and nacl August 4, 2021 19:47
nacl
nacl reviewed Aug 8, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@nacl nacl left a comment

Choose a reason for hiding this comment

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

I'm fine with the idea, but I think that this rule and the overall reasoning should be documented somewhere to reduce surprise during reviews. Could this be enforced somehow?

There are others that might be missing here. This one:

rules_pkg/pkg/tests/rpm/BUILD

Lines 286 to 291 in f7f2bd3

py_library(
name = "rpm_util",
srcs = ["rpm_util.py"],
imports = ["."],
visibility = [":__subpackages__"],
)
came to mind as I was browsing around.

@aiuto
Copy link
Copy Markdown
Collaborator Author

aiuto commented Aug 9, 2021

I'm fine with the idea, but I think that this rule and the overall reasoning should be documented somewhere to reduce surprise during reviews. Could this be enforced somehow?

There are others that might be missing here. This one:

rules_pkg/pkg/tests/rpm/BUILD

Lines 286 to 291 in f7f2bd3

py_library(
name = "rpm_util",
srcs = ["rpm_util.py"],
imports = ["."],
visibility = [":__subpackages__"],
)

came to mind as I was browsing around.

Right... the imports=[.] is a particular problem for me. It does not exist in Blaze py_library. I have to map it out on import.

@nacl nacl mentioned this pull request Aug 18, 2021
Open
nacl
nacl approved these changes Aug 18, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@nacl nacl left a comment

Choose a reason for hiding this comment

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

I don't want to block your importing into Google's repos. Feel free to submit.

Regardless, the import style should be documented and (preferably) enforced soon (#412).

@aiuto aiuto merged commit 0516f84 into main Aug 18, 2021
@aiuto aiuto deleted the imp branch August 18, 2021 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nacl nacl nacl approved these changes

@dannysullivan dannysullivan Awaiting requested review from dannysullivan

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@aiuto @nacl