Skip to content

Canonicalize targets#108

Merged
illicitonion merged 11 commits intobazel-contrib:mainfrom
darkrift:canonicalize_target
May 1, 2025
Merged

Canonicalize targets#108
illicitonion merged 11 commits intobazel-contrib:mainfrom
darkrift:canonicalize_target

Conversation

@darkrift
Copy link
Copy Markdown
Contributor

@darkrift darkrift commented Mar 24, 2025

This canonicalize all the labels in all attributes found that are label related using the output of bazel mod dump_repo_mapping "" for each commit.

This addresses #105

It doesn't have any integration tests yet because it would be much better to leverage #104 which is still in the works but it was tested against our internal monorepo and the reproducer in #105

Label like attributes (string defined but has nodep = True) are special and handled as an exception as if they were labels, and thus also converted. (See this thread)

@darkrift darkrift changed the title Canonicalize target Canonicalize targets Mar 25, 2025
@darkrift
Copy link
Copy Markdown
Contributor Author

darkrift commented Apr 1, 2025

@illicitonion if you could review this :)

@darkrift
Copy link
Copy Markdown
Contributor Author

@sitaktif Could I get a review on this ?

Is the lack of integration tests a blocker ? I'd really like to add some but at the same time, they are tedious to make and would be much easier to add if #104 was merged

Copy link
Copy Markdown
Collaborator

@sitaktif sitaktif left a comment

Choose a reason for hiding this comment

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

Code LGTM, thanks for the contribution 👍

Regarding the lack of integration test while #104 is open, what about you provide two states (e.g. two commits in a scratch repo?), ensuring that TD behaves correctly only after the patch? Then as part of #104 we can backport that test case.

@darkrift
Copy link
Copy Markdown
Contributor Author

@sitaktif
Copy link
Copy Markdown
Collaborator

sitaktif commented May 1, 2025

Ha, that is exactly what I meant, thanks for pointing it out.

I'm ok to merge 👍

@sitaktif
Copy link
Copy Markdown
Collaborator

sitaktif commented May 1, 2025

Please remember to squash your commits! :-)

@illicitonion illicitonion merged commit a6a2456 into bazel-contrib:main May 1, 2025
1 check passed
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