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: support cordova versioning #76

Closed

Conversation

azachar
Copy link

@azachar azachar commented Jul 26, 2016

Hello,
I have implemented Cordova versioning support. So it bumps versions to package.json and config.xml too.

This PR contains doc and tests.

Best regards,
Andrej

@coveralls
Copy link

coveralls commented Jul 26, 2016

Coverage Status

Coverage increased (+1.0%) to 95.918% when pulling dda5409 on azachar:cordova-support into 16f46cd on conventional-changelog:master.

@coveralls
Copy link

coveralls commented Jul 26, 2016

Coverage Status

Coverage increased (+1.0%) to 95.918% when pulling dda5409 on azachar:cordova-support into 16f46cd on conventional-changelog:master.

@bcoe
Copy link
Member

bcoe commented Jul 26, 2016

@azachar cool 👍 seems reasonable that we'd also bump the config.xml. Any thoughts, @stevemao, @Tapppi?

@stevemao
Copy link
Member

I haven't used cordova for a long time. Just thought people might ask for bumping bower.json, component.json or any other meta data.

@Tapppi
Copy link
Member

Tapppi commented Jul 27, 2016

This is cool, but I kind of worry about the same thing as @stevemao, in that how many different files we'll end up bumping. Would it be possible to make these plugins, or at least standardise the process of adding and testing different formats and files in the code to avoid spaghetti? There's also the burden of maintaining different formats if they happen to change, but I think that might be a moot point since version formats should rarely change. If it's structured / maintainable, I think this would be really cool though! 👍

@azachar
Copy link
Author

azachar commented Jul 27, 2016

Great to hear about you guys, I am on the same page with you. Perhaps my implementation could be already moved to a separate bump plugin:) Do you have some ideas how this can be elaborated? Would you be able to provide me with a plugin framework structure so it can be easily extended?

@bcoe
Copy link
Member

bcoe commented Jul 27, 2016

@Tapppi, @azachar I think it might be a bit premature to pull this into its own plugin system, but! I think it would be smart to pull the bumping logic into its own module, and I'd be curious to see how much logic we can re-use for the various meta-information files.

@gkatsev
Copy link

gkatsev commented Aug 26, 2016

bower actually doesn't recommend updating or using a version field in bower.json anymore. Since the versions are available as gittags. Though, I guess some people still use it, even though it's optional.

@Tapppi
Copy link
Member

Tapppi commented Aug 26, 2016

@gkatsev good point about bower, but not really an issue in this PR since it is about bumping the version number in config.xml. @stevemao @bcoe Is there a discussion to be had about the implications of adding supported files or should we move this forward? IMHO this is a fine change for now and the process of adding supported file formats is a discussion to be had alongside it in a separate issue (#100).

Copy link
Member

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

Thank you very much for the contribution @azachar, I've made a few comments, but in general I agree with @Tapppi that it would be great to support another platform.

Want to rebase with #114 once it lands, and we'll work on adding this?

@@ -7,7 +7,7 @@

> stop using `npm version`, use `standard-version` it rocks!

Automatic versioning and CHANGELOG management, using GitHub's new squash button and
NPM and Cordova Automatic versioning and CHANGELOG management, using GitHub's new squash button and
Copy link
Member

Choose a reason for hiding this comment

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

Rather than changing this copy, lets a section towards the end of the the README (below Badges!) that's something along the lines of:

Cordova Support

standard-version works with Cordova, and will automatically increment android-versionCode in your config.xml.

2. uses [conventional-changelog](https://github.com/conventional-changelog/conventional-changelog) to update _CHANGELOG.md_
3. commits _package.json_ and _CHANGELOG.md_
4. tags a new release
2. bumps the _version_ and _android-versionCode_ in _config.xml_ (based on your commit history)
Copy link
Member

Choose a reason for hiding this comment

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

let's keep the original copy here, and as suggested above add a section on Cordova (we can do the same if we add more technologies).

@@ -55,7 +56,7 @@ This has the benefit of making your repo/package more portable, so that other de
Install globally (add to your `PATH`):

```
npm i -g standard-version
npm i -g azachar/standard-version
Copy link
Member

Choose a reason for hiding this comment

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

remember to change this back.

var pkg = require(pkgPath)
var semver = require('semver')
var util = require('util')

function isCordovaProject () {
return !!fs.existsSync(cordovaPath)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should make this slightly stricter; check whether file exists, and also confirm that it contains android-versionCode perhaps? I can imagine projects having a config.xml that are not cordova.

@@ -23,6 +23,11 @@
"standard"
],
"author": "Ben Coe <ben@npmjs.com>",
"contributors": [
{
"name": "Andrej Zachar <andrej@chocolatejar.eu>"
Copy link
Member

Choose a reason for hiding this comment

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

I don't usually keep this updated; instead letting people click the contributors link in GitHub; if you'd like to toss your name here, mind also adding all the other folks listed under contributors; alternatively let's take this out 👍

var pkg = require(pkgPath)
var semver = require('semver')
var util = require('util')

function isCordovaProject () {
Copy link
Member

Choose a reason for hiding this comment

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

mind pulling these three new methods (isCordovaProject, andriodVersionCode, bumpCordova) into a library file:

lib/cordova

I think this would be a good way to divide things up, and would provide a clear path for other people adding new file formats.

execCli().code.should.equal(0)
var content = fs.readFileSync('config.xml', 'utf-8')
content.should.match(/version="1.2.4"/)
content.should.match(/android-versionCode="1020040"/)
Copy link
Member

Choose a reason for hiding this comment

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

love the tests!

@bcoe
Copy link
Member

bcoe commented Oct 6, 2016

@azachar still interested in contributing this patch?

@azachar
Copy link
Author

azachar commented Oct 9, 2016

Yep, I am, just busy with work and life :) I will come back to you next week.

@stevemao stevemao mentioned this pull request Oct 27, 2016
@bcoe
Copy link
Member

bcoe commented Nov 27, 2016

@azachar @Tapppi I've refactored @lexich's work on bower.json support, such that it provides a fairly generic foundation for adding more config file formats:

#148

@azachar if you want to work from this, I think it adds a good foundation for adding config.xml support; I'd recommend abstracting out the step that reads and writes the config into a helper function, then you can treat .xml fils differently than .json files.

@Tapppi
Copy link
Member

Tapppi commented Dec 1, 2016

@bcoe Thank you for making that happen, I should've written my comment on the changes here probably.. I think abstracting out the write step will be helpful, we'll see when someone has the time to pick this up 😄

@bcoe
Copy link
Member

bcoe commented May 5, 2017

@azachar would love to land this if you have any time to put in and still need this feature; closing ticket for now since we haven't had activity since December.

@bcoe bcoe closed this May 5, 2017
@Tapppi
Copy link
Member

Tapppi commented May 9, 2017

@bcoe im assigning myself to this, I want to tackle this and some other issues once I finally get some kind of a holiday (maybe one day...)

@Tapppi Tapppi self-assigned this May 9, 2017
@bcoe
Copy link
Member

bcoe commented May 9, 2017

@Tapppi awesome 👍 I'm actually off to climb in Yosemite for a few days ⛰ -- hoping to get some OSS work done at night.

@Tapppi
Copy link
Member

Tapppi commented May 9, 2017

Wrong thread -- off Topic. But that is frickin awesome. Yosemite is like top 5 on my list of places to visit. Big into bouldering and beatiful places 😉 have fun, I hear it's an amazing places!

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

6 participants