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

Switching to conventional commits #73

Closed
ROpdebee opened this issue Oct 2, 2021 · 5 comments
Closed

Switching to conventional commits #73

ROpdebee opened this issue Oct 2, 2021 · 5 comments
Labels

Comments

@ROpdebee
Copy link
Owner

ROpdebee commented Oct 2, 2021

I've been using a whole range of different commit message styles, and it makes me cringe a little each time. I should switch to conventional commits instead.

Pros:

  • Semantic commit messages
  • Would integrate nicely with a future Continuous Deployment workflow
  • Possibly changelogs?

Cons:

  • It's a change to the dev workflow, albeit IMO just a minor one
  • It'll take a bit of getting used to, but I'm sure we could set up a commit message linter in CI and/or a pre-commit hook to enforce it

Conventional commits is often paired with SemVer, but I have no intention of switching over to SemVer. It doesn't make sense for userscripts IMO, and that would lead to lots of issues with auto-updating unless you start with major version 2022. Regardless, there's still other benefits of conventional commits.

Not sure how we should handle scopes though. Currently I'm mostly annotating commits that touch a certain userscript with a sort-of arbitrary prefix (e.g. [caa upload] for ECAU), I think it'd make sense to carry that over into the scopes since this is a monorepo of different userscripts, after all. However, I'm not sure what'd be a good scope name for e.g. ECAU.

  • Using mb_enhanced_cover_art_uploads would lead to commit headers that are far too long (I don't like the look of feat(mb_enhanced_cover_art_uploads): Add some provider)
  • Using ecau feels like a bad idea because it's not obvious what ecau stands for, and I've only been using that initialism for just a couple of days in absense of a better shorthand
  • Continuing to use caa uploads would probably be the better option, but it kind of feels too "broad" of a name.

That's not even mentioning all the other scripts:

  • mb_blind_votes.user.js -> blind votes (seems OK)
  • mb_bulk_copy_work_codes.user.js -> work codes? A bit too broad, IMO.
  • mb_caa_dimensions.user.js -> caa dims (seems OK too)
  • mb_collapse_work_attributes.user.js -> Currently using collapse work attrs, but that's a bit long. Also have used coll work attrs but coll is ambiguous (could stand for collection too)
  • mb_multi_external_links.user.js -> No clue, none in use yet. split links?
  • mb_qol_inline_recording_tracks.user.js -> None yet.
  • mb_qol_seed_recording_disambiguation.user.js -> seed rec comments currently
  • mb_qol_select_all_update_recordings.user.js -> None yet.
  • mb_supercharged_caa_edits.user.js -> caa edits currently, a bit too similar to caa uploads IMO.
  • mb_validate_work_codes.user.js -> validate work attrs/validate work codes currently, too long. It may make sense to eventually merge this script with mb_bulk_copy_work_codes to create a larger script.

@kellnerd, since you're the only other person that's committing to this repo, your opinion matters a lot in this too.

@ROpdebee ROpdebee added the meta label Oct 2, 2021
@kellnerd
Copy link
Collaborator

kellnerd commented Oct 2, 2021

I am fine with that, I will just continue following the style of your commit messages as I (roughly) did before. The abbreviations which we are currently using as scope names look sensible to me, they don't have to be unambiguous globally. Although it has a slightly different meaning, what about hide work attrs instead of collapse work attrs? Simply work codes for a combined copy & validate work codes userscript is ok for me, especially as the collapsing of work attributes might be natively supported by MBS in the future.

Given the length restriction for the first line of the commit message, I usually prefer to keep the prefix as short as possible and only include the scope (if at all), especially since I consider most of my commits neither to be "fix" nor "feat", just me changing or enhancing something 😁
So we should definitely define a few short custom(?) types when we start using conventional commits.
And I would have to start using synonyms for "to fix" if you do not want all my bug fix commit messages to look like "fix(scope): Fix this and that" as I usually describe them.

@ROpdebee
Copy link
Owner Author

ROpdebee commented Oct 2, 2021

Well, there's many different types of changes, like here: https://github.com/conventional-changelog/commitlint/tree/master/%40commitlint/config-conventional so just changing some internals could just be "refactor(scope): yada yada", etc.

In terms of rewording the fix commits, I'd be fine with "fix(scope): this and that" (although arguably it should be two commits, "fix(scope): this" and "fix(scope): that" :P). Looking at the recent history, here's what it could look like:

  • [work codes] Support other languages on ISWCNet -> feat(work codes): support other languages on ISWCNet
  • [caa upload] Fix duplicate image detection on error -> fix(caa upload): duplicate image detection on error (or alternatively: fix(caa upload): allow retrying on error with additional details in the body. That message would better convey the actual fix IMO)

In any case, I wouldn't be too strict about it. These commit linter rules look mostly acceptable to me. I won't refuse to merge a PR just because someone wrote fix(scope): Fix stuff, or forgot to include the right scope*, or even when someone didn't use CC at all (although I'd probably point it out).

Commitizen might be a good idea too.

Anyways, I'll try it out once I start working on enhancements to ECAU, which will be after I'm done with #72

*Although, the latter one would depend on how this integrates with automated build tooling. If the build tools use the scope to determine whether something is up for a release, and lacking a scope causes them to misbehave, then I would probably require a rebase before merging (or just squash and merge and write a commit message myself).

@ROpdebee
Copy link
Owner Author

I've made the switch in recent commits, so closing this.

@ROpdebee ROpdebee pinned this issue Jun 13, 2022
@ROpdebee
Copy link
Owner Author

For the record, here's the scopes I've been using lately (mostly for my own reference, because I tend to forget):

scope project notes
ecau mb_enhanced_cover_art_uploads Used to be caa upload, somehow I switched to ecau. Might be useful to "expand" this scope if working on a specific provider/seeder (ecau/atisket, ecau/bandcamp)?
caa dims mb_caa_dimensions
split links mb_multi_external_links
seed disamb mb_qol_seed_disambiguation_comments
caa edits mb_supercharged_caa_edits
work codes mb_bulk_copy_paste_work_codes
lib Anything just affecting @src/lib

Ones to avoid, but have been used in the past:

  • qol: Too broad
  • build: There's a type for that.

@jesus2099
Copy link
Contributor

jesus2099 commented May 21, 2023

For what it's worth, I don't prefix the issues and commits with script names or codes.

For commits, I let the file list do it.
For the issues I use tags.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants