8000 TravisCI should help to discover eslint warnings. by AlexKVal · Pull Request #800 · react-bootstrap/react-bootstrap · GitHub
[go: up one dir, main page]

Skip to content

TravisCI should help to discover eslint warnings. #800

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

Merged
merged 1 commit into from
Jun 8, 2015

Conversation

AlexKVal
Copy link
Member
@AlexKVal AlexKVal commented Jun 8, 2015

Addresses such kind of things in review process:
#786 (comment)
#786 (comment)

@mathieumg
Copy link
Member

This would not only impact CI though. I don't have a strong opinion on whether they should all be warnings, all be errors or be a combination; I just wanted to point that out.

@mtscout6
Copy link
Member
mtscout6 commented Jun 8, 2015

This will be good in the long run for a cleaner code base. LGTM

@dozoisch
Copy link
Member
dozoisch commented Jun 8, 2015

Weird travis passed but not appveyor! But LGTM. It's important we keep the code clean

@AlexKVal
Copy link
Member Author
AlexKVal commented Jun 8, 2015

@mathieumg We anyway ask authors to address npm run lint warnings.
With this patch they will have immediate feedback from TravisCI-robot 👍

This will save their and our time in the long run 🍒

8000

@mtscout6
Copy link
Member
mtscout6 commented Jun 8, 2015

An interesting note about AppVeyor is that it is merging in master and testing that. Whereas travis does not.

@AlexKVal
Copy link
Member Author
AlexKVal commented Jun 8, 2015

appveyor fails with this (already addressed in another commit) warning:

src\Pagination.js
  37:28  error  hasHiddenPagesBefore is defined but never used  no-unused-vars

I merge.

AlexKVal added a commit that referenced this pull request Jun 8, 2015
TravisCI should help to discover eslint warnings.
@AlexKVal AlexKVal merged commit 1f7b9b2 into react-bootstrap:master Jun 8, 2015
@mathieumg
Copy link
Member

An interesting note about AppVeyor is that it is merging in master and testing that. Whereas travis does not.

I was wondering too, I'm no CI expert. How does Travis proceed? A PR is only a patch in the end, it has to be applied on something to be whole and testable.

I imagine this was an exceptional situation anyway, a PR which changes the conditions for a build to pass/fail submitted in-between two CI runs for another PR, which happens to break the build by the new conditions but not the old ones.

@mtscout6
Copy link
Member
mtscout6 commented Jun 8, 2015

😨 Bahh too much complexity....

I don't know the specifics, but I seem to recall that for every pull request github attempts to maintain two references to it. One which is the last commit of the PR and the second is the sha for a commit that would merge the two together. I believe you can fetch either of those commits to run tests against. This is a foggy recollection from something I read a long time ago.

@mathieumg
Copy link
Member

Oh, yeah, that's right. I checked out a PR once that way, had totally forgotten about it. It went something like /refs/pull/1234 where 1234 was the PR number. Thank you for the reminder! 👍

@mtscout6
Copy link
Member
mtscout6 commented Jun 8, 2015

Guess now is a good time for a quick plug to my blog. This is what I use to test PRs out locally: http://softwarebymatt.com/blog/git-pull-request-fetch/

@mathieumg
Copy link
Member

Awesome, added to my aliases! 🍭

@AlexKVal
Copy link
Member Author
AlexKVal commented Jun 8, 2015

If you are not the owner of the project your git req doesn't help.

I'm using git cou-pr 786 for all projects I contribute.
https://github.com/AlexKVal/dotfiles/blob/master/git/gitconfig.symlink#L13

There are git remote upstream master_github_url
and git remote origin pointing to my fork of that project.

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.

4 participants
0