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

clean up UUID #1028

Merged
merged 2 commits into from
Feb 6, 2023
Merged

clean up UUID #1028

merged 2 commits into from
Feb 6, 2023

Conversation

linzhp
Copy link
Contributor

@linzhp linzhp commented Jan 28, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

There are still some variables and functions with "UUID" in their name, although we no longer used UUID to identify py_library targets. The gazelle_deps macro still includes the google/uuid library.

What is the new behavior?

  • Change from names from "UUID" to more general "id" to hide implementation details of the id generation
  • bazel run //:gazelle_update_repos to clean up google/uuid library

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

  • bazel run //:gazelle_update_repos doesn't generate the load statement correctly. I had to manually revert it back.
  • We may also consider adding the py_library's module name to py_test and py_binary's deps, so it will be resolved just like regular imports and we no longer need any id for the py_library. The only problem is when a module is at the root of a Python project, the module name becomes empty, which is not valid.

@linzhp linzhp requested a review from f0rmiga as a code owner January 28, 2023 16:19
@linzhp linzhp marked this pull request as draft January 28, 2023 16:38
@linzhp
Copy link
Contributor Author

linzhp commented Jan 28, 2023

Don't review for now. I am exploring a different way for resolving same package imports

@linzhp
Copy link
Contributor Author

linzhp commented Jan 28, 2023

@f0rmiga Please review #1029 first. If that PR is OK, I will reduce this PR to only clean up the deps.bzl.

@linzhp linzhp marked this pull request as ready for review February 4, 2023 15:27
@linzhp
Copy link
Contributor Author

linzhp commented Feb 4, 2023

reduced this PR to only clean up the deps.bzl.

@f0rmiga f0rmiga merged commit 7948858 into bazelbuild:main Feb 6, 2023
@linzhp linzhp deleted the uuid branch February 6, 2023 05:12
ianpegg-bc pushed a commit to ianpegg-bc/rules_python that referenced this pull request May 12, 2023
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