prettier / prettier-eslint-cli

CLI for prettier-eslint
https://npm.im/prettier-eslint-cli
MIT License
539 stars 85 forks source link

feat: make the cli work with/without `prettier-eslint` peer #438

Closed JounQin closed 2 years ago

JounQin commented 2 years ago

What:

close #144, close #436

BREAKING CHANGE: bump all upgradable (dev)Dependencies except pure ESM

(WTF semantic-release...)

We must be careful on merging...

BREAKING CHANGE: token must be in the footer of the commit (which is stupid again and again)

Why:

close #144

How:

mark prettier-eslint as optional peer dependency, and fallback to internal @prettier/eslint which actually pointing to prettier-eslint

codesandbox-ci[bot] commented 2 years ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

codecov-commenter commented 2 years ago

Codecov Report

Merging #438 (6b41988) into master (58101c1) will not change coverage. The diff coverage is 100.00%.

:exclamation: Current head 6b41988 differs from pull request most recent head 2a7fb4d. Consider uploading reports for the commit 2a7fb4d to get more accurate results

@@            Coverage Diff            @@
##            master      #438   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         4    +1     
  Lines          161       163    +2     
  Branches        27        27           
=========================================
+ Hits           161       163    +2     
Impacted Files Coverage Δ
src/format-files.js 100.00% <ø> (ø)
src/prettier-eslint.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

idahogurl commented 2 years ago

@JounQin FYI, the release didn't work. semantic-release determined it didn't need to version bump. https://github.com/prettier/prettier-eslint-cli/runs/7808657314?check_suite_focus=true

[3:01:49 PM] [semantic-release] › ℹ  Start step "analyzeCommits" of plugin "@semantic-release/commit-analyzer"
[3:01:49 PM] [semantic-release] [@semantic-release/commit-analyzer] › ℹ  Analyzing commit: feat!: bump all upgradable (dev)Dependencies except pure ESM (#437)
[3:01:49 PM] [semantic-release] [@semantic-release/commit-analyzer] › ℹ  The commit should not trigger a release
JounQin commented 2 years ago

@JounQin FYI, the release didn't work. semantic-release determined it didn't need to version bump. https://github.com/prettier/prettier-eslint-cli/runs/7808657314?check_suite_focus=true

[3:01:49 PM] [semantic-release] › ℹ  Start step "analyzeCommits" of plugin "@semantic-release/commit-analyzer"
[3:01:49 PM] [semantic-release] [@semantic-release/commit-analyzer] › ℹ  Analyzing commit: feat!: bump all upgradable (dev)Dependencies except pure ESM (#437)
[3:01:49 PM] [semantic-release] [@semantic-release/commit-analyzer] › ℹ  The commit should not trigger a release

That's why I don't like it, I'd prefer changesets for releasing.

Any idea how can I fix? Or can we migrate to changesets?

cc @kylemh

kylemh commented 2 years ago

changesets wouldn't have magically resolved our issue either. In this situation, there's TECHNICALLY no breaking change for consumers because there's no interface change. That being said, I don't mind choosing a major release because we've changed how prettier-eslint resolves.

Instead of migrating to changesets we should figure out how to force a publish with a manually defined version for this one instance.

JounQin commented 2 years ago

changesets wouldn't have magically resolved our issue either. In this situation, there's TECHNICALLY no breaking change for consumers because there's no interface change. That being said, I don't mind choosing a major release because we've changed how prettier-eslint resolves.

Instead of migrating to changesets we should figure out how to force a publish with a manually defined version for this one instance.

changesets is based on changeset file, not relying on the commit message only. So there will be no issue if we migrate to changesets, because we can always add changeset file later before releasing.

Take https://github.com/prettier/eslint-plugin-prettier/commit/7bd70b65b680d360cd55aa9998804fc1e7188331#diff-8a555112659f40251b1022a16f7f984287f501c43766c71c0bfd2c2f3ef6ffa1R2 as an example.

kylemh commented 2 years ago

Oh damn. I WAS going to say: I'm not at all worried to switch to changesets. I love the tool! I'm simply saying this isn't really a good reason to switch away from semantic-release; however, I just googled and saw this:

You can trigger a release by pushing to your Git repository. You deliberately cannot trigger a specific version release, because this is the whole point of semantic-release.

That seems silly.


Okay, I don't know. Interested to hear what other say. The tool is technically correct. This isn't a major version change, but - in my opinion - versioning is best served as a communication tool with consumers. Making it a strict reference to changes in interface seems like losing out on an opportunity to be really responsible to our users.

JounQin commented 2 years ago

That seems silly.

Stupid, actually. 😁

Okay, I don't know. Interested to hear what other say. The tool is technically correct. This isn't a major version change, but - in my opinion - versioning is best served as a communication tool with consumers. Making it a strict reference to changes in interface seems like losing out on an opportunity to be really responsible to our users.

That's the point why I want to migrate to changesets! Right? 🤝

But to make it not a blocker, I'd like to mark a BREAKING CHANGE: commit in another PR.

kylemh commented 2 years ago

I like the non-blocking strategy personally, but will defer to others on that.

Alternative to changesets would be https://github.com/intuit/auto so that we can simply label changes whenever we want.

Either works well enough, imo.

JounQin commented 2 years ago

I like the non-blocking strategy personally, but will defer to others on that.

Yep, I'll raise a new PR requesting review later.

Alternative to changesets would be intuit/auto so that we can simply label changes whenever we want.

Either works well enough, imo.

I've been using changesets for a long time:

  1. we can define a custom/preset changelog format, including thanks mentions for contributors
  2. We can publish snapshot versions easily for testing before releasing via changesets-snapshot-action, although CodeSandbox CI has been doing this job

I'm not so familiar with https://github.com/intuit/auto.

idahogurl commented 2 years ago

My only concern with making it a peer dependency is that what if we make a breaking change where the CLI only works with pretter-eslint at some version.

JounQin commented 2 years ago

My only concern with making it a peer dependency is that what if we make a breaking change where the CLI only works with pretter-eslint at some version.

We can change the version range of peer.

idahogurl commented 2 years ago

Oh as for semantic-release vs changesets vs that Intuit one. I really don't have a preference. I use semantic-release for my VS Code extension and ended up doing what @kylemh is talking about by setting things. https://github.com/idahogurl/vs-code-prettier-eslint/blob/396efd13e65eb2cc624f9e354c0434ce504e9ea7/release.config.js#L8-L10

JounQin commented 2 years ago

Let's skip changesets in this PR and continue at https://github.com/prettier/prettier-eslint-cli/issues/441

@idahogurl Are we good to merge this PR?

github-actions[bot] commented 2 years ago

:tada: This PR is included in version 7.0.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: