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

Support for skipping lines before license header #1441

Merged
merged 14 commits into from
Jan 8, 2023

Conversation

abelk2
Copy link
Contributor

@abelk2 abelk2 commented Jan 2, 2023

This PR proposes a solution for supporting license headers on files where the header cannot be added immediately to the top of the file because of the format's restrictions. Examples are XML (<?xml version="1.0" ...) or shell scripts (#!/bin/bash).

The change simply allows configuring a regex for the LicenseHeaderStep. All the lines at the beginning of the file that match this regex will be skipped, and written directly to the output, then the rest of the lines will be processed the same way as before.

An example configuration would be something like:

format "licenseHeaderSh", {
  target "**/*.sh"
  licenseHeaderFile(file("$rootDir/config/license/sh.txt"), "^(?!##).+")
    .skipLinesMatching("^#!.+?\$")
}

This PR contains a working proof of concept, but lacks the following:

  • New test cases
  • Maven support
  • Change log updates

I'll add these if the concept is accepted.

Related issues:
#496

Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

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

This is a great proposal as-is, thanks very much! We will definitely merge this when you are ready :)

@nedtwigg
Copy link
Member

nedtwigg commented Jan 7, 2023

Run spotlessApply and commit so that the CI passes. Then update all three CHANGES.md , and also the README.md for plugin-maven and plugin-gradle.

@abelk2
Copy link
Contributor Author

abelk2 commented Jan 7, 2023

Run spotlessApply and commit so that the CI passes. Then update all three CHANGES.md , and also the README.md for plugin-maven and plugin-gradle.

Sure. Almost done, I also added a test case for it.
Let me add the README updates too and it's finished. :)

@abelk2 abelk2 marked this pull request as ready for review January 8, 2023 14:39
Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

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

This looks great and I think it's ready to merge! Do you agree that it's ready to merge @abelk2?

@abelk2
Copy link
Contributor Author

abelk2 commented Jan 8, 2023

This looks great and I think it's ready to merge! Do you agree that it's ready to merge @abelk2?

Yes, everything's ready :)

@nedtwigg nedtwigg merged commit 1effefb into diffplug:main Jan 8, 2023
benkard added a commit to benkard/mulkcms2 that referenced this pull request Apr 2, 2023
…2.0 (mulk/mulkcms2!16)

This MR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [com.diffplug.spotless:spotless-maven-plugin](https://github.com/diffplug/spotless) | build | minor | `2.31.0` -> `2.32.0` |

---

### Release Notes

<details>
<summary>diffplug/spotless</summary>

### [`v2.32.0`](https://github.com/diffplug/spotless/blob/HEAD/CHANGES.md#&#8203;2320---2023-01-13)

##### Added

-   Add option `editorConfigFile` for `ktLint` [#&#8203;142](diffplug/spotless#142)
    -   **POTENTIALLY BREAKING** `ktlint` step now modifies license headers. Make sure to put `licenseHeader` *after* `ktlint`.
-   Added `skipLinesMatching` option to `licenseHeader` to support formats where license header cannot be immediately added to the top of the file (e.g. xml, sh). ([#&#8203;1441](diffplug/spotless#1441)).
-   Add YAML support through Jackson ([#&#8203;1478](diffplug/spotless#1478))
-   Added support for npm-based [ESLint](https://eslint.org/)-formatter for javascript and typescript ([#&#8203;1453](diffplug/spotless#1453))
-   Better suggested messages when user's default is set by JVM limitation. ([#&#8203;995](diffplug/spotless#995))

##### Fixed

-   Support `ktlint` 0.48+ new rule disabling syntax ([#&#8203;1456](diffplug/spotless#1456)) fixes ([#&#8203;1444](diffplug/spotless#1444))
-   Fix subgroups leading catch all matcher.

##### Changes

-   Bump default version for `prettier` from `2.0.5` to `2.8.1` ([#&#8203;1453](diffplug/spotless#1453))
-   Bump the dev version of Gradle from `7.5.1` to `7.6` ([#&#8203;1409](diffplug/spotless#1409))
    -   We also removed the no-longer-required dependency `org.codehaus.groovy:groovy-xml`
-   Breaking changes to Spotless' internal testing infrastructure `testlib` ([#&#8203;1443](diffplug/spotless#1443))
    -   `ResourceHarness` no longer has any duplicated functionality which was also present in `StepHarness`
    -   `StepHarness` now operates on `Formatter` rather than a `FormatterStep`
    -   `StepHarnessWithFile` now takes a `ResourceHarness` in its constructor to handle the file manipulation parts
    -   Standardized that we test exception *messages*, not types, which will ease the transition to linting later on
    -   Bump default `ktlint` version to latest `0.47.1` -> `0.48.1` ([#&#8203;1456](diffplug/spotless#1456))
-   Switch our publishing infrastructure from CircleCI to GitHub Actions ([#&#8203;1462](diffplug/spotless#1462)).
    -   Help wanted for moving our tests too ([#&#8203;1472](diffplug/spotless#1472))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNC4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjQuMCJ9-->
@anantdamle
Copy link

anantdamle commented May 17, 2023

when following configuration with spotless 6.18.0

format "licenseHeaderSh", {
  target "**/*.sh"
  licenseHeader(licenseWithHash, "^(?!##).+")
    .skipLinesMatching("^#!.+?\$")
}

Results in

Skipping '/path/to/shell/script.sh' because it does not converge.

File sample:

#!/bin/bash

# Fail on any error.
set -e

# Display commands being run.
# WARNING: please only enable 'set -x' if necessary for debugging, and be very
#  careful if you handle credentials
set -x

# Code under repo is checked out to
cd my-dir

# build all of them, concurrency is unlimited, sync output
./gradlew clean test build

@abelk2
Copy link
Contributor Author

abelk2 commented May 17, 2023

@anantdamle Two things would help us to advise you more easily:

  • The contents of the licenseWithHash variable
  • The output of the spotlessDiagnose task (few of the files it outputs)

@nedtwigg
Copy link
Member

I'm pretty sure what's happening is this:

{file}

license header
{file}

license header
license header
{file}

license header
license header
license header
{file}

etc, and that's why it's not converging. If you'd like more help, please open an issue.

@anantdamle
Copy link

Thanks folks as requested by @abelk2 contents of licenseWithHash

#
# Copyright 2023 My Awesome Company
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#     https://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#

the output from Diagnose is different than @nedtwigg

license header

license header
...

{file}

@abelk2
Copy link
Contributor Author

abelk2 commented May 18, 2023

I think I see what causes the issue. First of all, Spotless has to tell somehow which line is the first one after the license header, i.e. where is the end of the license header if it's already in the file. A regex is used for this purpose, which has to match the first line that is not part of the license header.

I think you're using the one from the example ("^(?!##).+") which translates to "a line that does not start with double # symbols". Since your license header does not start with ##, but with a single # symbol, the header itself will match the regex. Because of this, the header is not replaced, but duplicated with each step, causing the divergence.

I'd recommend either:

  • Using ## prefix for the license header - preferred, this helps Spotless distinguish license headers from ordinary comments, or
  • Modifying the regex to "^(?!#).+" - however, please note that this will not match ordinary comments either, so comments immediately after the license header will be replaced by the header too

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