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

Promoted internal _tidy function to the utils module as dedent #793

Merged
merged 3 commits into from
Jun 27, 2021

Conversation

UebelAndre
Copy link
Collaborator

The functionality here would be useful elsewhere so I thought it should go in utils.bzl and I thought it'd be clearer to borrow the term dedent from python.

rust/private/utils.bzl Show resolved Hide resolved
@UebelAndre UebelAndre merged commit 55c46ee into bazelbuild:main Jun 27, 2021
@UebelAndre UebelAndre deleted the dedent branch June 27, 2021 18:48
@hlopko
Copy link
Member

hlopko commented Jun 28, 2021

Just a drive by - consider adding a test if you still have the behavior of the function cached in brain :) https://docs.bazel.build/versions/main/skylark/testing.html#for-testing-starlark-utilities

@UebelAndre
Copy link
Collaborator Author

Just a drive by - consider adding a test if you still have the behavior of the function cached in brain :) https://docs.bazel.build/versions/main/skylark/testing.html#for-testing-starlark-utilities

I'll add it to my list 😄

@UebelAndre
Copy link
Collaborator Author

Just a drive by - consider adding a test if you still have the behavior of the function cached in brain :) https://docs.bazel.build/versions/main/skylark/testing.html#for-testing-starlark-utilities

I feel like if this is going to be tested then it should go in bazel_skylib seems like functionality that would make folks happy. Or maybe even better, the doc attribute would be smart enough to perform a dedent, or maybe stardoc? It also feels kinda odd _tidy/dedent exists. We now pay a runtime cost to have the code look nicer? I'm all for maintainability but I think we could get away with writing maintainable code without dedent here. Thoughts? Should this be promoted? Should it be removed? Is this the thing that really needs tests (over others)?

@hlopko
Copy link
Member

hlopko commented Jun 30, 2021

First of all, writing a test is independent from (and maybe a prerequisite for) later points.

After we have an implementation that is field tested and of good quality I think folks at https://github.com/bazelbuild/bazel-skylib might accept it. Historically they've been very pedantic and strict about what goes into Skylib and I don't know the exect policy. We should try.

If you want to automatically dedent the string in doc attribute, you'll have to fix stardoc (somewhere here: https://osscs.corp.google.com/bazel/bazel/+/master:src/main/java/com/google/devtools/build/skydoc/SkydocMain.java;l=103?q=SkydocMain&ss=bazel). I don't think the runtime cost of dedenting is significant enough to worry about it. And if it is, the short term fix is to extract doc strings to constants and not having extra indentation inside.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants