Add Rewriter rule to position Package() after loads in BUILD file (#1138) #1139
Conversation
dnathe4th
requested review from
vladmos,
pmbethe09 and
meisterT
as code owners
|
@vladmos Can you take a look? |
| @@ -95,3 96,43 @@ func TestRewriterWithRewriter(t *testing.T) { | |||
| t.Error("Original Printer should not equal Modified Printer") | |||
| } | |||
| } | |||
|
|
|||
| func TestBUILDFileOrderingRewriter(t *testing.T) { | |||
There was a problem hiding this comment.
What's the reason this test is implemented separately, not as a test file under build/testdata? Rewrites are also done after ordinary formatting, aren't they?
|
Sorry for the delay. This is a backward-incompatible change (it may make some previously correctly formatted files not-formatted) and may require some repository cleanup, I'll do it in the near future (probably even merge this PR before and temporarily disable the reformatting rule internally). |
|
No worries on the delay, I understand we're all volunteers here :- ) Do you have a specific example of a correctly-formatted (i.e. |
|
Currently if |
|
Is that convention (package allowed to go before load) something I should
update this code to *not* reorganize? I am fine with that if it doesn't
actually impact the correctness of the final build file.
…On Tue, Mar 7, 2023 at 6:36 AM Vladimir Moskva ***@***.***> wrote:
Currently if package() goes before load() it's still correctly formatted
(it violates the convention that load() should go first, but buildifier
doesn't reorder the statements). After the change it will start reordering
them, making files that were previously considered as formatted
non-formatted. That can in theory block presubmits if there's a check that
buildifier doesn't modify BUILD files, even if the changes in the files are
competely irrelevant to load or package statements.
—
Reply to this email directly, view it on GitHub
<#1139 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACKQ7RSCF4WQLAZ6VEEMTDW25BYBANCNFSM6AAAAAAVFAF5IE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
No, by convention |
|
Could you use |
|
But the goal is for the formatting process to enforce the requirements of
the spec by default, not by opt-in. I understand this is a one-time
breaking change *for existing invalid build files*, but the goal is for
gazelle (and extensions) and buildifier and buildozer to have one fewer
thing to worry about from a file formatting perspective because this core
functionality is aligning the build file to the spec for them.
…On Wed, Mar 15, 2023, 8:32 AM Laurent Le Brun ***@***.***> wrote:
Could you use buildifier --lint=fix --warnings=load-on-top instead of
adding a new rewriter?
—
Reply to this email directly, view it on GitHub
<#1139 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACKQ7U2RTMFX6URHEU7V4DW4HOJZANCNFSM6AAAAAAVFAF5IE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Sure, but I'd (personally) prefer to see a pull request that makes I'll let @vladmos decide how to proceed (we need to take care of the one time migration of the Google codebase). |
|
The problem I was actually trying to solve for was more like
"package-between-load-and-rules".
Maybe you're saying the path to this is to make it a fix first, release
that so people can opt on, and then do a later release with it as the
default?
…On Wed, Mar 15, 2023, 9:02 AM Laurent Le Brun ***@***.***> wrote:
Sure, but I'd (personally) prefer to see a pull request that makes
load-on-top on by default, instead of implementing a new rewriter.
I'll let @vladmos <https://github.com/vladmos> decide how to proceed (we
need to take care of the one time migration of the Google codebase).
—
Reply to this email directly, view it on GitHub
<#1139 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACKQ7WMKEKMWNJBZVZW7ATW4HRYTANCNFSM6AAAAAAVFAF5IE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
@sdtwigg is handling the google internal migration to package after load but before any rules. FWIW, we are also well on the way towards making package(default_attr=something) apply to the entire file and not just following rules. That is a breaking change but results in the least surprise for users. The result is that if buildifier starts moving packages around, it will either be a logical no-op, OR will expose behavior which will soon break anyway. |
This change adds a Rewrite rule to enforce the position of the
package()function invocation within a BUILD file. This function must be called after all of theload()s, but before any rule, according to the documentation.Making this change here in the Rewriter also simplifies gazelle extensions manipulating this function in BUILD files as they no longer need to inspect the rest of the file for positional information.