Skip to content

feat: allow using both prettier and standardjs - #5679

Merged
ferrarimarco merged 1 commit into
mainfrom
remove-js-corner-cases
Jul 19, 2024
Merged

ferrarimarco merged 1 commit into
mainfrom
remove-js-corner-cases

Conversation

@ferrarimarco

@ferrarimarco ferrarimarco commented May 24, 2024 •

Copy link
Copy Markdown
Collaborator

Proposed changes

  • Allow using both Prettier and StandardJS at the same time by removing
    JAVASCRIPT_DEFAULT_STYLE and TYPESCRIPT_DEFAULT_STYLE
    configuration variables. They create a confusing corner case that
    Super-linter already handles with its core logic that's in place to
    enable and disable linters.
  • Document VALIDATE_JAVASCRIPT_PRETTIER and VALIDATE_TYPESCRIPT_PRETTIER
    because they were missing from the README.
  • Add missing TYPESCRIPT_PRETTIER tests. We didn't notice it was missing because we weren't running TYPESCRIPT_PRETTIER at all.

See the Upgrade guide for guidance about how to update your Super-linter configuration.

Readiness checklist

In order to have this pull request merged, complete the following tasks.

Pull request author tasks

  • I included all the needed documentation for this change.
  • I provided the necessary tests.
  • I squashed all the commits into a single commit.
  • I followed the Conventional Commit v1.0.0 spec.
  • I wrote the necessary upgrade instructions in the upgrade guide.
  • If this pull request is about and existing issue,
    I added the Fix #ISSUE_NUMBER label to the description of the pull request.

Super-linter maintainer tasks

  • Label as breaking if this change breaks compatibility with the previous released version.
  • Label as either: automation, bug, documentation, enhancement, infrastructure.

@ferrarimarco ferrarimarco added enhancement New feature or request breaking big changes, potentially causes failure O: backlog 🤖 Backlog, stale ignores this label labels May 24, 2024
@ferrarimarco ferrarimarco self-assigned this May 24, 2024
@ferrarimarco ferrarimarco changed the title feat!: deprecate ts and js language config vars Deprecate JavaScript and TypeScript language selection config variables May 24, 2024
@ferrarimarco ferrarimarco added this to the 7.0.0 milestone May 24, 2024
@ferrarimarco
ferrarimarco force-pushed the remove-js-corner-cases branch from 7923111 to af4026d Compare May 24, 2024 12:28
@ferrarimarco
ferrarimarco marked this pull request as draft May 24, 2024 16:01
@ferrarimarco
ferrarimarco force-pushed the remove-js-corner-cases branch from af4026d to 8efbb5b Compare May 25, 2024 06:58
@ferrarimarco
ferrarimarco force-pushed the remove-js-corner-cases branch 6 times, most recently from 1f20935 to ec450c5 Compare July 11, 2024 11:21
@ferrarimarco
ferrarimarco marked this pull request as ready for review July 11, 2024 13:39
@ferrarimarco
ferrarimarco force-pushed the remove-js-corner-cases branch 4 times, most recently from 9763a5b to a3c69e4 Compare July 18, 2024 08:26
@ferrarimarco ferrarimarco changed the title Deprecate JavaScript and TypeScript language selection config variables feat: allow using both prettier and standardjs Jul 18, 2024
@ferrarimarco ferrarimarco modified the milestones: 7.0.0, 6.9.0, 6.8.0 Jul 18, 2024
@ferrarimarco

Copy link
Copy Markdown
Collaborator Author

@zkoppert can you please review this one? Thanks a lot!

@zkoppert

Copy link
Copy Markdown
Member

Document VALIDATE_JAVASCRIPT_PRETTIER and VALIDATE_TYPESCRIPT_PRETTIER
because they were missing from the README.
Add missing TYPESCRIPT_PRETTIER tests. We didn't notice it was missing because we weren't running TYPESCRIPT_PRETTIER at all.

😅

Comment thread test/linters/typescript_prettier/typescript_good_1.ts
zkoppert
zkoppert previously approved these changes Jul 18, 2024

@zkoppert zkoppert left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

✨

@ferrarimarco
ferrarimarco added this pull request to the merge queue Jul 18, 2024
@ferrarimarco
ferrarimarco removed this pull request from the merge queue due to a manual request Jul 18, 2024
- Allow using both Prettier and StandardJS at the same time by removing
  JAVASCRIPT_DEFAULT_STYLE and TYPESCRIPT_DEFAULT_STYLE
  configuration variables. They create a confusing corner case that
  Super-linter already handles with its core logic that's in place to
  enable and disable linters.
- Document VALIDATE_JAVASCRIPT_PRETTIER and VALIDATE_TYPESCRIPT_PRETTIER
  because they were missing from the README.
- Add missing TYPESCRIPT_PRETTIER tests.
@ferrarimarco

Copy link
Copy Markdown
Collaborator Author

sorry, forgot to fix the upgrade guide :)

@ferrarimarco
ferrarimarco added this pull request to the merge queue Jul 19, 2024
Merged via the queue into main with commit 2daf461 Jul 19, 2024
@ferrarimarco
ferrarimarco deleted the remove-js-corner-cases branch July 19, 2024 05:17
@gavllew

gavllew commented Aug 1, 2024

Copy link
Copy Markdown

This was correctly labelled as a breaking change, as it means that super-linter behaviour changes unless you update the configuration options, and yet it was released without a major version bump. This is a shame; I'm sure a lot of CI runs will currently be failing due to the change.

The changes also mean that with default configuration, both the standard and prettier formatters are enabled. I don't think these two have compatible styles, so doesn't that mean all JS/TS files will fail linting by default?

@ferrarimarco

Copy link
Copy Markdown
Collaborator Author

Hi!

You might have a point here, although the reason why we didn't mark this as a breaking change is that enabling this behavior has always been possible, but it wasn't documented.

The fact that they didn't run at the same time created a confusing corner case, which we classified as a bug, as you can also see from the fact that we were missing both documentation and a test case.

@gavllew

gavllew commented Aug 1, 2024

Copy link
Copy Markdown

The thing that makes it a "confusing corner case" is that standard and prettier are formatters, not linters. It makes sense to run lots of linters to pick up issues in a variety of file types. IMO it never makes sense to run multiple formatters (for the same file type), as they are not going to agree on the correct style - if they did then one of them would be redundant. The old config option made much more sense to me that what was introduced here.

Layer on top of that the fact it breaks existing configuration without a major version bump, and probably breaks out-of-the-box default configuration for JS/TS files, then my suggestion would be to roll back the change and re-consider how and when the changes should be re-introduced.

@ferrarimarco

Copy link
Copy Markdown
Collaborator Author

Again, you have a point in suggesting that users might want to run only one formatter per file type.

The confusing corner case I'm referring to is that, before this change, to do that for Standard and Prettier, you would need to set the JAVASCRIPT_DEFAULT_STYLE and TYPESCRIPT_DEFAULT_STYLE variables, while all other tools (both linters and formatters) that Super-linter ships use VALIDATE_xxxx variables for that. After this change, you can use VALIDATE_JAVASCRIPT_PRETTIER, VALIDATE_JAVASCRIPT_STANDARD, VALIDATE_TYPESCRIPT_PRETTIER, and VALIDATE_TYPESCRIPT_STANDARD for the same purpose, bringing this in line with what you do with other linters and formatters.

The issue, in this case, is that the VALIDATE_JAVASCRIPT_PRETTIER, VALIDATE_JAVASCRIPT_STANDARD, VALIDATE_TYPESCRIPT_PRETTIER, and VALIDATE_TYPESCRIPT_STANDARD variables were breaking the behavior described in the Configure Super-linter section of the readme, reported here for convenience:

The VALIDATE_[LANGUAGE] variables work as follows:
super-linter runs all supported linters by default.
If you set any of the VALIDATE_[LANGUAGE] variables to true, super-linter defaults to leaving any unset variable to false (only validate those languages).
If you set any of the VALIDATE_[LANGUAGE] variables to false, super-linter defaults to leaving any unset variable to true (only exclude those languages).
If you set any of the VALIDATE_[LANGUAGE] variables to both true and false, super-linter fails reporting an error.

This is why this change was necessary.

The points you're raising warrant an addition to the upgrade guide, though. I'll provide it in a separate PR.

@ferrarimarco

Copy link
Copy Markdown
Collaborator Author

Thanks for raising this @gavllew . I just sent #5948 to help with the configuration update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking big changes, potentially causes failure enhancement New feature or request O: backlog 🤖 Backlog, stale ignores this label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants