Skip to content

Comments

Use treefmt to nixpkgs-fmt all .nix files#2831

Merged
flokli merged 16 commits intodevelopfrom
treefmt-nixpkgs-fmt
Nov 8, 2022
Merged

Use treefmt to nixpkgs-fmt all .nix files#2831
flokli merged 16 commits intodevelopfrom
treefmt-nixpkgs-fmt

Conversation

@flokli
Copy link
Contributor

@flokli flokli commented Nov 8, 2022

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@flokli flokli temporarily deployed to cachix November 8, 2022 11:44 Inactive
@flokli flokli changed the title Treefmt nixpkgs fmt Use treefmt to nixpkgs-fmt all .nix files Nov 8, 2022
@flokli flokli temporarily deployed to cachix November 8, 2022 11:48 Inactive
@flokli flokli temporarily deployed to cachix November 8, 2022 11:48 Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Nov 8, 2022
@flokli flokli temporarily deployed to cachix November 8, 2022 11:51 Inactive
@flokli flokli temporarily deployed to cachix November 8, 2022 11:51 Inactive
@flokli flokli force-pushed the treefmt-nixpkgs-fmt branch from 3a0f550 to b3443c9 Compare November 8, 2022 12:04
@flokli flokli temporarily deployed to cachix November 8, 2022 12:05 Inactive
@flokli flokli temporarily deployed to cachix November 8, 2022 12:05 Inactive
Copy link
Member

@jschaul jschaul left a comment

Choose a reason for hiding this comment

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

Looks ok modulo the github actions block.

Copy link
Member

Choose a reason for hiding this comment

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

This github action is unnecessary, as linters already run make lint-all here: https://github.com/zinfra/cailleach/blob/ab3a921c7a6cc903c0d1d578040e05a566c84bfa/wire-server-private/ci/pipelines/jobs/linters.dhall#L40-L60

please remove this github actions block. Also, we agreed to not introduce more github actions if not strictly necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to keep these 20 lines of yaml.

  • They're not a big maintenance burden, as it simply invokes treefmt after installing the dependencies.
  • Very quick feedback (40 seconds)
  • No requirement to log into concourse, so useful feedback for external contributors

It would be a different thing if we'd start to model complicated logic in GH actions, but this simply executes a script to give semi-instant feedback.

It looks like Concourse also does it, because it's invoked in the lint-all target from the Makefile, but it's way less accessible.

Copy link
Member

Choose a reason for hiding this comment

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

it spams emails and re-does something that is already done, giving twice the amount of linting feedback for the same issue. To me, this is just noise and unnecessary. Maybe someone else has an opinion here also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can change if GH actions should send you emails on failure in your notification settings:

image

@flokli flokli force-pushed the treefmt-nixpkgs-fmt branch from b3443c9 to 0058d72 Compare November 8, 2022 12:58
@flokli flokli temporarily deployed to cachix November 8, 2022 12:58 Inactive
@flokli flokli temporarily deployed to cachix November 8, 2022 12:58 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants