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

chore(build): travis-after-all #3

Closed
wants to merge 1 commit into from
Closed

Conversation

sarbbottam
Copy link
Owner

using travis-after-all npm module

@codecov-io
Copy link

Current coverage is 100.00%

Branch #3 has no coverage reports uploaded yet.

No diff could be generated. No reports for master found.
Review entire Coverage Diff as of 17778da

Powered by Codecov. Updated on successful CI builds.

@sarbbottam
Copy link
Owner Author

@ta2edchimp could you take a look?

@@ -6,7 +6,7 @@ cache:
notifications:
email: false
node_js:
- '4'
- stable
- '0.10'
before_install:
- npm i -g npm@^3.0.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd definitely test on npm@^2.0.0 too. There seems to be a whole lot of devs still using not only node@0.1x, but also npm@^2.0.0. On ghooks I introduced a script that decides which version of npm to install based on the current node version. (This is mainly because of something I can only describe as a bug, in npm@2.15.x and I think it would be good to cover such cases as early as possible.)

@ta2edchimp
Copy link
Collaborator

Boy, am I nitpicking (I just learned that word) 😜

Other than my notes 👍

@ta2edchimp
Copy link
Collaborator

Maybe, when merging, from now on we could mostly use the "squash and merge" feature? I think we both still have lots of ideas for this module, and that way we'd keep the commit history relatively clean and short.

@ta2edchimp
Copy link
Collaborator

I guess this one will trigger a new release, the first release, because #2 did not yet.
Prior to merging we should do another PR, that fixes the README (badges, description, acknowledgement to the original module).
Oh, and change the repo's description, too.

@sarbbottam
Copy link
Owner Author

@ta2edchimp, I have addressed the review comments. Please have a look when you get time.


My 💵 0.02 for the other suggestions

I think we both still have lots of ideas for this module, and that way we'd keep the commit history relatively clean and short.

👍

... "squash and merge" feature

  • I generally keep single commit : single use case
  • when addressing review comments, most of the time I do git all; git ci --amend, if the suggestions are related to the last commit
  • if the review comments are not related to the last commit them
    • add a dummy commit
    • git rebase -i HEAD~(sequence number of the related commit)
    • and quash the related ones, basically using fix instead of pick

Prior to merging we should do another PR, that fixes the README (badges, description, acknowledgement to the original module).

Absolutely, will hold on to this, till the suggested change are done.

If you get time feel free to raise PR for the same, Otherwise I would do it tomorrow morning, quite busy schedule today.

BTW, you have full access to the repo.

@sarbbottam
Copy link
Owner Author

Should we disable release for the repo now and enable it only after

fixing the README (badges, description, acknowledgement to the original module).

using travis-after-all npm module
@ta2edchimp
Copy link
Collaborator

Should we disable release for the repo now and enable it only after

fixing the README (badges, description, acknowledgement to the original module).

I guess so. Would be a nice clean new release then 👍


Re ... "squash and merge" feature: seems we understand each other 😉


I'm afraid I won't have much time until later at the weekend as it's birthday week in the family. But depending on how much time you got for this, mind beginning posting issues for the features we came along in the last days? We then might pick us some to realize or discuss those that need it.

Btw. do you know git-labelmaker? Could come handy ...

@sarbbottam
Copy link
Owner Author

Btw. do you know git-labelmaker? Could come handy

Nope, looks great!


This PR disables the semantic release.

npm run travis-after-all
"travis-after-all": "travis-after-all && npm run report-coverage"

Do you think its good to be merged?


If OK I would create another PR from this branch after renaming, there is a typo and the name is misleading. thi PR has nothing to do with semantic release

@ta2edchimp
Copy link
Collaborator

LGTM 👍

@sarbbottam
Copy link
Owner Author

closed in favor of #4

@sarbbottam sarbbottam closed this Apr 8, 2016
@sarbbottam sarbbottam deleted the sematic-release branch April 13, 2016 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants