manolo / gwt-api-generator

Generator for creating GWT JSInterop clients from Polymer Web Components
Apache License 2.0
50 stars 24 forks source link

Use OSS parent and package.json info for deploying #33

Closed manolo closed 9 years ago

manolo commented 9 years ago

Review on Reviewable

Saulis commented 9 years ago

Reviewed 4 of 4 files at r1. Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


gulpfile.js, line 207 [r1] (raw file): Should this be fs.existsSync(parent) ?


Comments from the review on Reviewable.io

manolo commented 9 years ago

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


gulpfile.js, line 207 [r1] (raw file): sure


Comments from the review on Reviewable.io

Saulis commented 9 years ago

I would like us to remove the version usage from package.json, otherwise it looks good. (would drop and squash some commits to remove changes that have been reverted in a newer version)


Reviewed 1 of 4 files at r1, 1 of 2 files at r3, 3 of 3 files at r4. Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.


gulpfile.js, line 210 [r4] (raw file): I wouldn't use pkg.version at all, it's a lot clearer and simpler just to use either --version argument or the default value of 1.0-SNAPSHOT


gulpfile.js, line 211 [r4] (raw file): I would remove this also.


template/pom.template, line 11 [r4] (raw file): I would use <%= version %> instead.


template/tasks/global-variables.js, line 16 [r2] (raw file): I would leave this and use it in the pom.


Comments from the review on Reviewable.io

Saulis commented 9 years ago

Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.


template/pom.template, line 22 [r4] (raw file): I would change org to organization to reflect pom.xml and usage better.


Comments from the review on Reviewable.io

manolo commented 9 years ago

I'd rather to use version from package.json because it's clearer if we take a look to it. Otherwise we have to maintain the current version in the postinstall script command line which is pretty much difficult to read.


Review status: 3 of 4 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.


gulpfile.js, line 210 [r4] (raw file): I have to disagree, the package.json version is very clear about its meaning what is the evolution of the product, otherwise we have to change the --version argument in the postinstall line which is not clear at all.


gulpfile.js, line 211 [r4] (raw file): version field in package.json is very restrictive to n.n.n so we have to use another field for release info. We could use a customised field like: libraryversion or pomversion or whatever, but that might end misaligning version and pomversion


template/pom.template, line 22 [r4] (raw file): done


Comments from the review on Reviewable.io

Saulis commented 9 years ago

Reviewed 1 of 4 files at r1, 1 of 2 files at r3, 2 of 3 files at r4, 1 of 1 files at r5. Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.


gulpfile.js, line 211 [r4] (raw file): I agree that it's clearer to use a version from the package.json instead of a postinstall script. However the current mismatch we have between two ways of versioning is quite confusing. For example the version number you end up currently in gwt-polymer-elements after running npm install is 1.0.2.0.alpha1-SNAPSHOT I don't think anybody would use such a version number.

I would be OK with a solution where we use the version from package.json and append it with either .0 or -SNAPSHOT which ever makes more sense. But we get rid of the release custom property.


template/pom.template, line 11 [r4] (raw file): Regarding my last comment, -SNAPSHOT should be appended into version if args.version is undefined


template/tasks/global-variables.js, line 15 [r2] (raw file): I would leave version here and assign either args.version or pkg.version+ -SNAPSHOT to it.


Comments from the review on Reviewable.io

manolo commented 9 years ago

Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.


gulpfile.js, line 211 [r4] (raw file): No idea what you want, sorry.


template/pom.template, line 11 [r4] (raw file): SNAPSHOT is mandatory in maven projects as convention at least when you use release plugin. It's not a package.json feature and makes not sense to have it anywhere else. It's only used if you release daily snapshots.


Comments from the review on Reviewable.io

Saulis commented 9 years ago

Reviewed 1 of 2 files at r3, 1 of 3 files at r4, 2 of 2 files at r6, 1 of 1 files at r7. Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io