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

feat: custom 'bumpFiles' and 'packageFiles' support #372

Merged
merged 8 commits into from Dec 6, 2019
Merged

Conversation

jbottigliero
Copy link
Member

@jbottigliero jbottigliero commented Jun 4, 2019

This is a POC for allowing version bumps in custom bumpFiles. It moves packageFiles and bumpFiles to command line "arguments" ...but the configuration for bumpFiles is better served as part of a configuration file.

I've added a test showing how it can be used to support a basic mix.exs file (mentioned in #313).

I'm not super happy with the option ({filename: string, replacer: Regex}), but it seemed like a decent place to get the conversation started (since we'll likely want to get something added to the conventional-changelog-config-spec before anything changes here).

Ideally, I'd like to come up with a single solution that solves for #313, #322 and #348.

Some alternatives I've considered:

  • glob – I'm not sure it'd be possible to get the replacement we want.
  • Custom Callback – this would probably be the most flexible and require the least amount of support on our end going forward. Basically, rather than replacer: Regex, we could process a handler: Function option (this also assumes yargs.config will be ok with aFunction).
  • Semantic Version Detection – I considered using something like semver to detect version... but this seems extremely error-prone and less precise than alternatives.

@jbottigliero jbottigliero requested a review from bcoe Jun 4, 2019
@coveralls
Copy link

@coveralls coveralls commented Jun 4, 2019

Coverage Status

Coverage decreased (-0.5%) to 99.42% when pulling 3be2404 on feat-bump-files into ecf26b6 on master.

@bcoe
Copy link
Member

@bcoe bcoe commented Jun 4, 2019

@jbottigliero 👋 I've been at a variety of conferences for the past couple weeks, sorry for the slow reviews.

Thanks for the hard work you've been putting into standard-version, I will make an effort to give you feedback soon.

Are you in the slack, you should feel free to interact with me there (it's a good way to help keep me in line 😉 ).

@caiorcferreira
Copy link

@caiorcferreira caiorcferreira commented Jun 9, 2019

First of all, thanks for the great tool!

What is the status of this feature? It would really help me to use this tool with a gradle-based project.

If has anything that I can do to help this, please let me know 😁

@bcoe
Copy link
Member

@bcoe bcoe commented Jun 9, 2019

@jbottigliero I really like this work, it's not dissimilar to some work I'm doing here:

https://github.com/googleapis/release-please/tree/master/src/updaters

To make version updates more extensible; perhaps it would make sense to do something similar, where we have some default updating patterns, but we can also extend on the logic used to update a version for edge-case file formats?

@caiorcferreira I'm a proponent of this feature 👌 @jbottigliero what could you use help with to see it over the finish line?

@jbottigliero
Copy link
Member Author

@jbottigliero jbottigliero commented Jun 11, 2019

@bcoe I think at this point it's really it's just coming up with an acceptable API for folks.

I dig the updaters implementation, but I think I've already hit a bit of a limitation. Take a mix.exs file for example... the user can technically define their version in a number of ways:

defmodule Phoenix.MixProject do
  use Mix.Project

  @version "1.5.0-dev"

  def project do
    [
      app: :phoenix,
      version: @version,

phoenixframework/phoenix

defmodule ExercismTestRunner.Mixfile do
  use Mix.Project

  def project do
    [
      app: :tests,
      version: "0.0.1",

exercism/elixir

It's probably best we don't make any assumptions about these files. Maybe we could offer out-of-the-box solutions for the most common scenarios, while allowing customization.

Maybe rather than {filename: string, replacer: Regex} configuration I initially proposed, we could support {filename: string, updater: string | { readVersion: Function, writeVersion: Function} }?

Where the string variant would be expected to be a path, and the Object version implies .js configuration support.

@jbottigliero
Copy link
Member Author

@jbottigliero jbottigliero commented Jun 11, 2019

I've made some tweaks to the proposal – the object will just support strings for both filename and updater (expecting a file path to a require-able file that exports a readVersion and writeVersion).

If this direction seems reasonable, I think as far as what is left:

  • Consider adding the option to the spec.
  • Add documentation (README) about officially supported and custom updaters/package files.

I'm also considering using the package-files nomenclature everywhere. To me, the next step here is to update the initial version reading from any package-file using readVersion.

Taking a look at index.js, that means we'll just want to expose/expect an isPrivate function.

@jbottigliero
Copy link
Member Author

@jbottigliero jbottigliero commented Jun 12, 2019

Support for a VERSION file was mentioned over in Slack and it has me thinking that the updaters/package-files might be better organized by file content classification(s) rather than filename.

Basically, what I've proposed as the package-files/version.txt.js package file is probably better classified as package-files/plain-text.js. We could then have some of the most common filenames mapped similar to how the package-files/json.js, but this might allow for more flexibility for end consumers.

{
  bumpFiles: [
    'version.txt',
    'VERSION',
    {
      filename: 'other-plain-text-file-that-is-in-my-repo.txt',
      // Not sure the best way to express this in the configuration,
      // but I think this describes my intent.
      updater: '@standard-version/package-files/plain-text' 
      // Maybe just using `type` here would resolve to internal updaters instead, ie.
      // type: 'plain-text'
      // ...this would reserve `updater` for custom updaters.
    }
  ]
}

@silasb
Copy link

@silasb silasb commented Jul 24, 2019

@jbottigliero this is awesome. I've been using it for the past day or so and it's been working good for me.

Used this to installe it:

 yarn global add conventional-changelog/standard-version.git#feat-bump-files

@bcoe
Copy link
Member

@bcoe bcoe commented Jul 29, 2019

@jbottigliero this API is looking great to me I'll make an effort to give a more thorough review soon? I'd say go ahead and start working on an updated README though?

@esciara
Copy link

@esciara esciara commented Jul 30, 2019

Exactly what I am looking for... Working on a python project, and that would help! I have not yet found a workaround to do this, even using bump2version in the workflow. (cannot pass on the previous version and current version from standard-version)

Actually would this allow to setup the files to bump and the text to look for?

bump2version does it like this, as stated in the doc:

Given this requirements.txt::

Django>=1.5.6,<1.6
MyProject==1.5.6

using this .bumpversion.cfg will ensure only the line containing MyProject will be changed::

[bumpversion]
current_version = 1.5.6

[bumpversion:file:requirements.txt]
search = MyProject=={current_version}
replace = MyProject=={new_version}

Can be multiple lines, templated using Python Format String Syntax.

@@ -307,69 +322,111 @@ standardVersion({
})
```

## Commit Message Convention, at a Glance
Copy link
Member Author

@jbottigliero jbottigliero Aug 7, 2019

Choose a reason for hiding this comment

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

I've removed this section – the README is pretty big and it seems like this is better handled by linking out to conventionalcommits.org.

lib/updaters/index.js Outdated Show resolved Hide resolved
index.js Outdated
let pkg
bump.pkgFiles.forEach((filename) => {
args.packageFiles.forEach((filename) => {
Copy link
Contributor

@regseb regseb Aug 7, 2019

Choose a reason for hiding this comment

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

@jbottigliero If the list of package files is configurable, updaters should be allowed as for bump files. Otherwise, this configuration will not work:

"packageFiles": [
    "pom.xml"
],
"bumpFiles": [
    {
        "filename": "pom.xml",
        "updater": "xml-updater.js"
    }
]

Copy link
Member Author

@jbottigliero jbottigliero Aug 24, 2019

Choose a reason for hiding this comment

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

Updated - thanks!

@jbottigliero jbottigliero changed the title wip: feat: custom 'bumpFiles' and 'packageFiles' support feat: custom 'bumpFiles' and 'packageFiles' support Aug 24, 2019
@jbottigliero
Copy link
Member Author

@jbottigliero jbottigliero commented Aug 24, 2019

Alright! I believe this is good to go... I'm not super happy with the API (when you have a custom packageFile and bumpFile the configuration seems a little verbose), so if you have any thoughts let me know!

Semi-related, also considering adding an examples directory with some common configurations that might help folks navigate the space based on all the updates we've been making.

@bcoe
Copy link
Member

@bcoe bcoe commented Aug 26, 2019

@tommywo enlist your feedback on this? You've been doing a lot of work on conventional changelog, and trust your opinion.

@FixRM
Copy link

@FixRM FixRM commented Sep 4, 2019

Hello guys, @jbottigliero. Personally I think this solution is really universal but it's may be "to match". I think it's ok to have unnecessary package.json file for non js projects. Actually we need it any way to install and run standard-version. I think we can use npx run instead of npm run but I'm not sure if it help standard-version to run our custom js scripts. We still need a place to reference custom script dependencies.

In other hand Regex probably be common used option for version update. So I think everyone will be happy to have package.json (unnecessary or not) as single source of true for the version number and a number of {filename: string, replacer: Regex} options to sync version number in other files. Custom scripts will be welcomed as well of course.

@jbottigliero
Copy link
Member Author

@jbottigliero jbottigliero commented Sep 4, 2019

@FixRM – thanks for the feedback! I moved away from the replacer option based on the updaters being both customizable and simple out-of-the-box solution. It does seem like we could support a regex type out of the box that would replace the replacer option – I'll explore this as it might help validate this API.

I haven't had the chance to test this out, but I'm hoping to port a few non-JS projects to use npx standard-version and .versionrc avoiding a JS file in the repository altogether.

@FixRM
Copy link

@FixRM FixRM commented Sep 24, 2019

Hello guys, any update on this?

@bcoe
Copy link
Member

@bcoe bcoe commented Sep 27, 2019

@FixRM 👋 hello, I've been a bit buried in issues lately and unfortunately haven't had much time for standard-version, @jbottigliero anyone else you think we could loop in for code review? I think this is looking great.

@jbottigliero
Copy link
Member Author

@jbottigliero jbottigliero commented Sep 28, 2019

@FixRM I've opened a PR (#457) with the type: regex support, I'm thinking it might make sense to continue the conversation there around that functionality specifically... I think it also shows the complexity around the replacer RegExp functionality in general and why this PR was rethought a bit.

@Heziode
Copy link

@Heziode Heziode commented Nov 23, 2019

Any progress ?

@NickDJM
Copy link

@NickDJM NickDJM commented Dec 4, 2019

Tested the branch with a custom YAML parser and it works amazingly!

Would be great to see this get merged in and released

@jbottigliero
Copy link
Member Author

@jbottigliero jbottigliero commented Dec 5, 2019

Hey folks, it looks like this is getting some traffic again – it's great to hear it's been working in the wild. I've rebased with the master and resolved conflicts.

I'd be more than happy to have this land in the next version. I'll need to coordinate with @bcoe or @tommywo on the release.

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

Successfully merging this pull request may close these issues.

None yet