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

compose with generator-travis #176

Merged

Conversation

@iamstarkov
Copy link
Contributor

@iamstarkov iamstarkov commented Oct 1, 2015

I found composeWith feature as awesome one and think that if yeoman community will have smaller generators then, everybody will benefit from it. For example with this pull-request, generator-node will no longer need to handle travis config on its own. What do you think?

@iamstarkov iamstarkov force-pushed the iamstarkov:feat/compose-with-generator-travis branch 2 times, most recently from 054af1d to ef0d087 Oct 1, 2015
@SBoudrias
Copy link
Member

@SBoudrias SBoudrias commented Oct 1, 2015

LGTM!

I just want to take a look at the travis generator before actually merging. I'll try to get back to you before the weekend.

this.composeWith('node:travis', {}, {
local: require.resolve('../travis')
this.composeWith('travis', {}, {
local: require.resolve('generator-travis/generators/app')

This comment has been minimized.

@arthurvr

arthurvr Oct 1, 2015
Member

Don't change the indentation here please :)

@arthurvr
Copy link
Member

@arthurvr arthurvr commented Oct 1, 2015

LGTM. Thanks!

Although it works for our use case, I think the generator-travis misses a bunch of things. It would be cool if it had options like what node versions to use, if people even want node at all, custom commands, ... Without those things it's useless for a bunch of generators out there.

@SBoudrias
Copy link
Member

@SBoudrias SBoudrias commented Oct 2, 2015

I have the same question as @arthurvr here, what is the plan for the future of this generator?

@iamstarkov
Copy link
Contributor Author

@iamstarkov iamstarkov commented Oct 4, 2015

what is the plan for the future of this generator?

It is really good question. I spend few days on thinking how to keep balance between single responsibility and extensibility.

Initial idea was to have easy way to get travis config with up to date node versions, like yo travis. Main improvement I can see is to extend existing travis config, (right now it is overriding every time).

I can easily support all the other fields, but I feel like it will blur purpose of this generator. If someone want to have other scripts in travis config, maybe its the job for generator-coveralls or smth. So this generator will only handle node versions and keep already existing fields as before, while other generators will be able to make their job. What do you think?

PS. the reason why I add nodejs as default env, because nodejs community is almost the only yeoman consumer.

PS 2. This Semver upgrade model seems good enough:

  1. Patch versions will be updated when generator is about to get small code fixes, or going to remove old node versions.
  2. Minor version will be updated, when generator will get new node versions in the list.
  3. Major versions will be updated, when generator will get breaking change for generator-consumers
@iamstarkov
Copy link
Contributor Author

@iamstarkov iamstarkov commented Oct 5, 2015

Second thought is to support all custom fields, because then everybody still can use up-to-date node versions and just extend travis config. So right now roadmap looks like this: 1. extend travis config 2. support all custom fields. What do you think?

@iamstarkov
Copy link
Contributor Author

@iamstarkov iamstarkov commented Oct 5, 2015

Delivered with generator-travis 1.1. Will add tests for extend logic as well

@iamstarkov
Copy link
Contributor Author

@iamstarkov iamstarkov commented Oct 5, 2015

only one problem I found, fields in result travis config is not sorted properly. For example:

writing: function () {
  this.composeWith('travis', { options: {
    config: { after_script: 'npm run coveralls' }
  }}, {
    local: require.resolve('generator-travis/generators/app')
  });
},

Will be:

after_script: 'npm run coveralls'
language: node_js
node_js:
  - stable
  - iojs
  - '0.12'
  - '0.10'

Is there any way to sorted JSON.stringify?

@iamstarkov
Copy link
Contributor Author

@iamstarkov iamstarkov commented Oct 5, 2015

Yay, I imlemented sorted Yaml config and added tests. Take a look pls.

@iamstarkov
Copy link
Contributor Author

@iamstarkov iamstarkov commented Oct 7, 2015

I’m also thinking to add feature of removing old node versions from the list, because whole point is to get "up-to-date" travis config, it will be true by default with possibility to turn off in options or with flag.

So what do you think about pull-request?

@iamstarkov
Copy link
Contributor Author

@iamstarkov iamstarkov commented Oct 9, 2015

@SBoudrias @arthurvr looking for your feedback =)

@SBoudrias
Copy link
Member

@SBoudrias SBoudrias commented Oct 9, 2015

I’m also thinking to add feature of removing old node versions from the list, because whole point is to get "up-to-date" travis config, it will be true by default with possibility to turn off in options or with flag.

IMO, I'd settle on a default config (0.10, 0.12, stable) and allow an option to manually specify versions. I'd be simpler.

It's kind of important currently supporting 0.10 to 4.1 because apt-get default is still 0.10, so a lot of people still have it installed.


For this CR, I'm okay to merge. But fix the indentation issue and update the readme too as there's references to the travis sub-generator.

@iamstarkov iamstarkov force-pushed the iamstarkov:feat/compose-with-generator-travis branch from b9cef50 to c8581b1 Oct 14, 2015
@iamstarkov
Copy link
Contributor Author

@iamstarkov iamstarkov commented Oct 15, 2015

  • fixed indentation /cc @arthurvr
  • update generator-node readme /cc @SBoudrias
  • add ability to specify nodejs versions in the config options /cc @SBoudrias

Also I wanted the more clear way to understand how to add or remove versions in the list, so I found nodejs/LTS plan and ended up relevant section in the generator-travis’s readme. generator-travis seems to be finished. What do you think?

@SBoudrias
Copy link
Member

@SBoudrias SBoudrias commented Oct 15, 2015

Cool, LGTM.

Please squash your commits and we'll merge.

@iamstarkov iamstarkov force-pushed the iamstarkov:feat/compose-with-generator-travis branch 2 times, most recently from 69ac994 to 8899491 Oct 15, 2015
@iamstarkov iamstarkov force-pushed the iamstarkov:feat/compose-with-generator-travis branch from 8899491 to c26bda8 Oct 15, 2015
@iamstarkov
Copy link
Contributor Author

@iamstarkov iamstarkov commented Oct 15, 2015

done

SBoudrias added a commit that referenced this pull request Oct 15, 2015
…ravis

compose with generator-travis
@SBoudrias SBoudrias merged commit 41642ee into yeoman:master Oct 15, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@SBoudrias
Copy link
Member

@SBoudrias SBoudrias commented Oct 15, 2015

Here it goes! Thanks a ton

@iamstarkov iamstarkov deleted the iamstarkov:feat/compose-with-generator-travis branch Oct 15, 2015
@hemanth
Copy link
Member

@hemanth hemanth commented Dec 3, 2015

👍

@iamstarkov I understand have dependencies is good thing, but would it be more cleaner if this lands in as travis with all the other generators ?

Say if someone wants to fiddle with travis generator, will land to the generators dir and wonders where the generator is and then realises it's from require.resolve('generator-travis/generators/app') and then has to do a PR to that generator....

//cc @SBoudrias

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.