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

Tweaks to scalafmt rules/script to support bazel test wrappers #1344

Conversation

virusdave
Copy link
Contributor

Relates to #994

With these changes, I can use a macro to place a wrapping sh_test
target around this script, and change the phase ordering to put
phase_scalafmt before the runfiles phase, and suddenly it works
as intended!

NB: If this change is acceptable to folks, then it might be worth
changing the default phase placement for ext_scalafmt to place
it before runfiles by default.

Example use:

In custom bzl file, I have:

_enable_format_targets_and_tests_by_default = True

_ext_scalafmt = dicts.add(
    _base_ext_scalafmt,
    {"attrs": dicts.add(
        _base_ext_scalafmt["attrs"],
        {"format": attr.bool(
            default = _enable_format_targets_and_tests_by_default,
            doc = "Enable the check-format and auto-format synthetic run targets (foo.format-test and foo.format respectively)",
        )},
    )},
    {"phase_providers": [
        "@//path/to/some/buildfile:phase_scalafmt",
    ]},
)

scala_library = make_scala_library(_ext_scalafmt)  # etc for other rule types

def maybe_add_format_test(**kwargs):
    if (kwargs.get("format", _enable_format_targets_and_tests_by_default)):
        name = kwargs["name"]
        sh_test(
            name = "{}.format-test.target".format(name),
            srcs = [":{}.format-test".format(name)],
            data = [
                ":{}".format(name),
            ],
            local = True,
        )

def _scalafmt_singleton_implementation(ctx):
    return [
        _ScalaRulePhase(
            custom_phases = [
                ("-", "runfiles", "scalafmt", _phase_scalafmt),
            ],
        ),
    ]

scalafmt_singleton = rule(
    implementation = _scalafmt_singleton_implementation,
)

then in path/to/some/buildfile/BUILD.bazel, I have:

load("//path/to/above/my_rules.bzl", "scalafmt_singleton")

scalafmt_singleton(
    name = "phase_scalafmt",
    visibility = ["//visibility:public"],
)

Description

Motivation

@virusdave virusdave force-pushed the dnicponski/scratch/tweak_scalafmt_testing branch from 9971079 to bf25d5f Compare February 5, 2022 01:18
Copy link
Collaborator

@liucijus liucijus left a comment

Choose a reason for hiding this comment

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

Thanks, @virusdave

@liucijus
Copy link
Collaborator

liucijus commented Feb 8, 2022

@virusdave please fix formatting for:
./scala/private/phases/phase_scalafmt.bzl # reformat

You can run ./lint.sh to get it fixed

With these changes, I can use a macro to place a wrapping `sh_test`
target around this script, and change the phase ordering to put
`phase_scalafmt` before the `runfiles` phase, and suddenly it works
as intended!

NB: If this change is acceptable to folks, then it might be worth
changing the default phase placement for `ext_scalafmt` to place
it before `runfiles` by default.

Example use:

In custom bzl file, I have:

```
_enable_format_targets_and_tests_by_default = True

_ext_scalafmt = dicts.add(
    _base_ext_scalafmt,
    {"attrs": dicts.add(
        _base_ext_scalafmt["attrs"],
        {"format": attr.bool(
            default = _enable_format_targets_and_tests_by_default,
            doc = "Enable the check-format and auto-format synthetic run targets (foo.format-test and foo.format respectively)",
        )},
    )},
    {"phase_providers": [
        "@//path/to/some/buildfile:phase_scalafmt",
    ]},
)

scala_library = make_scala_library(_ext_scalafmt)  # etc for other rule types

def maybe_add_format_test(**kwargs):
    if (kwargs.get("format", _enable_format_targets_and_tests_by_default)):
        name = kwargs["name"]
        sh_test(
            name = "{}.format-test.target".format(name),
            srcs = [":{}.format-test".format(name)],
            data = [
                ":{}".format(name),
            ],
            local = True,
        )

def _scalafmt_singleton_implementation(ctx):
    return [
        _ScalaRulePhase(
            custom_phases = [
                ("-", "runfiles", "scalafmt", _phase_scalafmt),
            ],
        ),
    ]

scalafmt_singleton = rule(
    implementation = _scalafmt_singleton_implementation,
)
```

then in `path/to/some/buildfile/BUILD.bazel`, I have:

```
load("//path/to/above/my_rules.bzl", "scalafmt_singleton")

scalafmt_singleton(
    name = "phase_scalafmt",
    visibility = ["//visibility:public"],
)
```
@virusdave virusdave force-pushed the dnicponski/scratch/tweak_scalafmt_testing branch from f8ec4b2 to e8f2d21 Compare February 8, 2022 17:05
@virusdave
Copy link
Contributor Author

@virusdave please fix formatting for: ./scala/private/phases/phase_scalafmt.bzl # reformat

You can run ./lint.sh to get it fixed

Done! Thanks.

@virusdave
Copy link
Contributor Author

Thoughts on this?

NB: If this change is acceptable to folks, then it might be worth
changing the default phase placement for ext_scalafmt to place
it before runfiles by default.

If it's agreed upon, i'm happy to make a followup change to do so.

@liucijus
Copy link
Collaborator

liucijus commented Feb 8, 2022

NB: If this change is acceptable to folks, then it might be worth
changing the default phase placement for ext_scalafmt to place
it before runfiles by default.

Yeah, help here is appreciated. Thanks for taking this! Do you mean tweaking what happens with make_scala_library?

@liucijus liucijus merged commit e942d94 into bazelbuild:master Feb 8, 2022
@virusdave
Copy link
Contributor Author

NB: If this change is acceptable to folks, then it might be worth
changing the default phase placement for ext_scalafmt to place
it before runfiles by default.

Yeah, help here is appreciated. Thanks for taking this! Do you mean tweaking what happens with make_scala_library?

I meant this: #1347

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