buildtools icon indicating copy to clipboard operation
buildtools copied to clipboard

WIP : Support Tabular Data formatting in Buildifier

Open SKashyap opened this issue 4 years ago • 13 comments

What?

This change adds to the Buildifier the ability to format tabular data using a set of new tags as follows :

Format tabular data in proper visual alignment #buildifier: format table Sort tabular data (by column) alongside the allignment #buildifier: format table sort <column-num>

Why?

The need for the change can be seen with the following example : Use an input file :

tableEx = [
    ("col1",      "col2",     "col3"),
    ("col1111", "col222", "col3333")    
]

Run it through buildifier to see the following output :

tableEx = [
    ("col1","col2","col3"),
    ("col1111","col222","col3333")    
]

Detailing the problem statement :

  • Tabular Data can be either "List of Tuples"/ "List of Lists". (See ListExpr , TupleExpr)
  • The buildifier's parser stage freezes how an Expr will be printed using flags file ForceCompaction for Tuples , and ForceMultiline for Lists.
  • These fixed notions of handling the print logic make it difficult to operate with tabular data as shown in the above example.
  • Even if the user formats the table himself, the buildfier cannot be indicated "#buildifer : Don't mess with my table". It ruins the user-provided format too. #leaveAlone does not work either.
  • We came across this problem when we were building large RPMs with 200 endstates, each having a location of install/permissions, etc as table data.

How?

Design Description :

  • Add support for 2 new tags to be parsed with rewrite.go : #buildifier: format table #buildifier: format table sort <column-num>
  • Existing architecture of buildifier allows rewriting the AST within rewrite.go based on parsed buildifier tags.
  • Utilize the same pipeline to "mark" the nodes to form the TableRoot and the TableRows based on the presence of the new tags.
  • Within print.go, flex the logic to allow printing a tuple /List in a tabular form when they are identified a TableRoot/TableRow node.

Printing of Tabular data :

  • We choose an existing golang package called TabWriter to jump-start us into this feature without writing custom logic to format rows/column sizes out the output.
  • TabWriter implements "Elastic Tabstops algorithm "

Other approaches evaluated :

  • We also evaluated an approach of creating a new Expr class called TableListExpr, TableRowExpr - It did not provide any advantage over the existing design given the current problem. We can revisit this later if the use cases demand it.

Sorting of Tabular data :

  • Rewrite.go logic where we detect the new tags also sort the row entries based on the index provided in the tag.

Future implementations:

  • This changeset only supports a "List of Tuples" as a table. A similar extension can be made for "List of List" as well.

Testing?

  • 15 testcases added to support the use-cases
  • bazel test ... : PASSED.

Any other info?

There are a couple of known drawbacks due to the TabWriter behavior :

  • Especially in the case of "Before Comments" in tableRows. We will be curating them into a document for reviewers soon.

SKashyap avatar Jun 23 '21 01:06 SKashyap

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla[bot] avatar Jun 23 '21 01:06 google-cla[bot]

@googlebot I fixed it.

rtabassum avatar Jun 23 '21 20:06 rtabassum

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla[bot] avatar Jun 23 '21 20:06 google-cla[bot]

@googlebot I fixed it.

SKashyap avatar Jun 24 '21 07:06 SKashyap

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla[bot] avatar Jun 24 '21 07:06 google-cla[bot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla[bot] avatar Jun 24 '21 07:06 google-cla[bot]

@googlebot I fixed it.

SKashyap avatar Jun 24 '21 08:06 SKashyap

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla[bot] avatar Jun 24 '21 08:06 google-cla[bot]

@googlebot I fixed it.

SKashyap avatar Jun 24 '21 15:06 SKashyap

@SKashyap looks like a really nice improvement! What's the status of this?

Gormo avatar Feb 02 '22 09:02 Gormo

Hey Simon,

Glad to hear that this improvement has caught your interest. This has been in WIP for too long. I am wanting to wrap a couple of golang nits in this change and submit soon.

From: Simon Björklén @.> Reply-To: bazelbuild/buildtools @.> Date: Wednesday, February 2, 2022 at 1:29 AM To: bazelbuild/buildtools @.> Cc: Shwetha Kashyap @.>, Mention @.***> Subject: Re: [bazelbuild/buildtools] WIP : Support Tabular Data formatting in Buildifier (#985)

@SKashyaphttps://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FSKashyap&data=04%7C01%7Ckshwetha%40vmware.com%7C4e58386848b844e8653908d9e62e70c1%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637793909414101677%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=NZx%2BtYbIIt9J3tfi9r6cYVNLLaKN1FyQSUnk8lKgiVc%3D&reserved=0 looks like a really nice improvement! What's the status of this?

— Reply to this email directly, view it on GitHubhttps://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fbazelbuild%2Fbuildtools%2Fpull%2F985%23issuecomment-1027741805&data=04%7C01%7Ckshwetha%40vmware.com%7C4e58386848b844e8653908d9e62e70c1%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637793909414101677%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=7Dp0dWxvUhSBs8NFykWRuXbzGfWkSawXh1LCd%2Fl4Go4%3D&reserved=0, or unsubscribehttps://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAAYIC4FKDQIN4GITVK5H753UZD2NTANCNFSM47EZLGAQ&data=04%7C01%7Ckshwetha%40vmware.com%7C4e58386848b844e8653908d9e62e70c1%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637793909414101677%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=NM%2BZOT%2FtRDxy0y3fc8AA%2FnoA8JHl8HfEh%2Fj89MnXvfI%3D&reserved=0. Triage notifications on the go with GitHub Mobile for iOShttps://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fapps.apple.com%2Fapp%2Fapple-store%2Fid1477376905%3Fct%3Dnotification-email%26mt%3D8%26pt%3D524675&data=04%7C01%7Ckshwetha%40vmware.com%7C4e58386848b844e8653908d9e62e70c1%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637793909414101677%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=aRyzJjaIPOzTlk0UkosQk%2Bqna4gSKCMXplZB8SlETSg%3D&reserved=0 or Androidhttps://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fplay.google.com%2Fstore%2Fapps%2Fdetails%3Fid%3Dcom.github.android%26referrer%3Dutm_campaign%253Dnotification-email%2526utm_medium%253Demail%2526utm_source%253Dgithub&data=04%7C01%7Ckshwetha%40vmware.com%7C4e58386848b844e8653908d9e62e70c1%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637793909414101677%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=pEOl0Hl2ofTTaBIGJKFJSdcEKV5FiX58AyfxUsVbQfs%3D&reserved=0. You are receiving this because you were mentioned.Message ID: @.***>

SKashyap avatar Feb 02 '22 16:02 SKashyap

@SKashyap have you given up on PR ?

systemlogic avatar Jul 04 '23 04:07 systemlogic

@SKashyap have you given up on PR ?

I had stopped working on this for a while. But have made some recent changes to this in our internal repository. Will collate those and wrap this asap.

SKashyap avatar Jul 04 '23 05:07 SKashyap