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

Silently omit git argument when git is unavailable #176

Merged
merged 3 commits into from
Feb 26, 2018

Conversation

nirizr
Copy link
Contributor

@nirizr nirizr commented Feb 22, 2018

Tries to fix #174

Alternatively, we could require a flag or take the test out of the git_info method although it seems fitting imo.

@GitCop
Copy link

GitCop commented Feb 22, 2018

There were the following issues with your Pull Request

  • Commit: 9f2ccfc
  • Commits must be in the following format: %{type}(%{scope}): %{description}

Guidelines are available at https://github.com/coveralls-clients/coveralls-python


This message was auto-generated by https://gitcop.com

@nirizr
Copy link
Contributor Author

nirizr commented Feb 23, 2018

Failures within the ci/circleci: check test seem unrelated. I think this PR is ready for review.

@jvarho
Copy link
Contributor

jvarho commented Feb 23, 2018

Would it make sense to make this an option like --no-git instead?

That would make it explicit that you intended it and didn't e.g. accidentally run outside the repo. You'd probably want a warning from this check that mentions the options in that case.

@nirizr
Copy link
Contributor Author

nirizr commented Feb 23, 2018

tbh I'm not sure the risk/damage of accidentally proceeding without git is high. worst thing you'll get something over to coveralls instead of nothing. Definitely warrants a warning though.

What do you think about adding a warning but keeping it the default to silently proceed without git? Perhaps a --require-git to turn the warning into an error instead?

lmk what you think is best and i'll have a commit doing whatever you pick.

Copy link
Owner

@TheKevJames TheKevJames left a comment

Choose a reason for hiding this comment

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

I personally think the flag would be overkill, but if you could please log a warning in the case where git support is not detected, that would be great.

coveralls/api.py Outdated
run_command('git', 'status')
except CoverallsException:
return {}

rev = run_command('git', 'rev-parse', '--abbrev-ref', 'HEAD').strip()
Copy link
Owner

Choose a reason for hiding this comment

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

Could you please put the try catch around the already-existing commands? No need to run an extra git command here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that; the original reason I ended up adding a git status command was to avoid any possible errors related specifically to the existing commands, that may be because of something other than git not being available.

I can't think of a good scenario but an unlikely example could be having a git version that does not support the current command (or any future changes). Either because git version is too old or too new with backwards incompatible cli changes.

WDYT?

Copy link
Owner

Choose a reason for hiding this comment

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

That makes sense -- I think if we add the logging, something like "could not collect git data -- are you running coveralls in a repository?", along with logging the CoverallsException message (which contains the process output), that should cover it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Will amend with a more verbose log (and phrasing that's more to your liking).

@TheKevJames
Copy link
Owner

Ah, and as per the tests -- so long as only the unused-variable for __class__ is failing, that's no problem. Known issue with pylint, I will update it outside of this PR.

@GitCop
Copy link

GitCop commented Feb 23, 2018

There were the following issues with your Pull Request

  • Commit: 017382d
  • Commits must be in the following format: %{type}(%{scope}): %{description}

Guidelines are available at https://github.com/coveralls-clients/coveralls-python


This message was auto-generated by https://gitcop.com

@GitCop
Copy link

GitCop commented Feb 23, 2018

There were the following issues with your Pull Request

  • Commit: e215097
  • Invalid type. Valid types are feat, fix, docs, style, refactor, perf, test, chore, revert

Guidelines are available at https://github.com/coveralls-clients/coveralls-python


This message was auto-generated by https://gitcop.com

@nirizr
Copy link
Contributor Author

nirizr commented Feb 23, 2018

Pinging @TheKevJames @jvarho, any additional thoughts/comments?

@TheKevJames
Copy link
Owner

@nirizr still would like to see you wrap the other git commands rather than introducing a new one. Looks like your log formatting is a bit janky -- you have a bracket-interpolation param with no matching data being passed in. Go ahead and take that out and just throw a log.exception(e) after your warning line.

@nirizr
Copy link
Contributor Author

nirizr commented Feb 23, 2018

Apologies, I thought you agreed with the git status thing, removed.
Sorry again for missing the formatting.

I used the exc_info optional argument instead of adding a log.exception so the exception info will be included in the warning. lmk if you want an log.exception instead or in addition to the log.warning.

Copy link
Owner

@TheKevJames TheKevJames left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks for the fix! I will cut a new release as soon as I can.

@TheKevJames TheKevJames merged commit f9db83c into TheKevJames:master Feb 26, 2018
@nirizr
Copy link
Contributor Author

nirizr commented Feb 26, 2018

Splendid! thanks for the quick responses!

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.

Don't assume running in git directory
4 participants