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.

Sign up for GitHub

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

Jump to bottom

docs command: don't show error on console when not a git repository #3700

Merged
merged 2 commits into from
Apr 28, 2018
Merged

docs command: don't show error on console when not a git repository #3700

merged 2 commits into from
Apr 28, 2018

Conversation

makenowjust
Copy link
Contributor

@makenowjust makenowjust commented Dec 15, 2016 •
edited
Loading

Currently, we get git error message when crystal docs run not inside git repository:

$ crystal docs
fatal: Not a git repository (or any of the parent directories): .git

It is unkind. This pull request fixes to output warning message instead of git error.

Sija
Sija reviewed Dec 15, 2016
View reviewed changes
src/compiler/crystal/tools/doc/generator.cr Outdated
puts "WARNING there is not inside git worktree. some features are not available."
return
end

remotes = `git remote -v`
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's enough to replace it with git remote -v 2>/dev/null?

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 don't know about git remote -v perfectly. Maybe it causes an error event inside git work-tree. I think we should use git rev-parse --is-inside-work-tree because it is official way to check whether inside git work-tree.

Copy link
Contributor

Choose a reason for hiding this comment

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

git remote returns list of remotes with remote url after name (because of verbose -v switch). Adding 2>/dev/null will silence error while leaving status code intact, thus triggering return on the next line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

git remote -v error message except fatal: Not a git repository may be useful for us, should not silence it. So, we should detect whether inside work-tree before git remote.

@makenowjust makenowjust changed the title Detect whether inside git worktree then output warning Detect whether inside git work-tree then output warning Dec 15, 2016
@makenowjust makenowjust force-pushed the fix/doc/check-git-worktree branch from 89d48e8 to 68d011e Compare December 15, 2016 10:09
@makenowjust makenowjust changed the title Detect whether inside git work-tree then output warning Detect whether inside git repository then output warning Dec 15, 2016
@makenowjust makenowjust force-pushed the fix/doc/check-git-worktree branch from 68d011e to 4f511a1 Compare December 15, 2016 10:11
@makenowjust makenowjust changed the title Detect whether inside git repository then output warning Show warning message if crystal docs cannot detect github repository Dec 15, 2016
@makenowjust makenowjust force-pushed the fix/doc/check-git-worktree branch from 4f511a1 to 6777410 Compare December 15, 2016 10:13
@makenowjust makenowjust changed the title Show warning message if crystal docs cannot detect github repository Show warning message if cannot detect github repository on crystal docs Dec 15, 2016
asterite
asterite reviewed Dec 15, 2016
View reviewed changes
src/compiler/crystal/tools/doc/generator.cr Outdated
@@ -20,6 +20,9 @@ class Crystal::Doc::Generator
@types = {} of Crystal::Type => Doc::Type
@repo_name = ""
compute_repository
if @repo_name == ""
puts "WARNING cannot detect GitHub repository. some features are not available."
Copy link
Member

Choose a reason for hiding this comment

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

I'd use a comma: "...repository, some features ..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed it.

@asterite
Copy link
Member

Thank you!

I wonder if we should show a warning or just silently do nothing if a git repo isn't found. You won't get links to the source code, but since that only works with github... if you are not using github then there isn't anything public to show links to, so saying nothing seems to be OK.

(eventually we could add links to bitbuckets and other repositories)

What do you think?

@makenowjust makenowjust force-pushed the fix/doc/check-git-worktree branch from 6777410 to 8c8c095 Compare December 17, 2016 05:44
straight-shoota
straight-shoota reviewed Oct 11, 2017
View reviewed changes
src/compiler/crystal/tools/doc/generator.cr
@@ -278,6 +281,10 @@ class Crystal::Doc::Generator
end

def compute_repository
# check whether inside git work-tree
`git rev-parse --is-inside-work-tree >/dev/null 2>&1`
Copy link
Member

Choose a reason for hiding this comment

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

Is this additional command really necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The below git remote -v will output an error message without this. This is needed.

@asterite
Copy link
Member

This PR is stalled. I don't know what needs to be done to continue.

If there's no activity, it will be closed in two weeks.

@makenowjust makenowjust force-pushed the fix/doc/check-git-worktree branch from 8c8c095 to f51de4e Compare April 28, 2018 02:20
@makenowjust
Copy link
Contributor Author

Sorry for delay. I'm sure preventing raw git error message is good, but I'm not sure it should show warning message. crystal docs can get link to not only GitHub but also GitLab now, and supported services may be added in future. Based on this, I now think it is not so important to show warning message.

@asterite asterite requested a review from RX14 April 28, 2018 02:41
@asterite asterite changed the title Show warning message if cannot detect github repository on crystal docs docs command: don't show error on console when not a git repository Apr 28, 2018
@RX14 RX14 added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler topic:tools labels Apr 28, 2018
@RX14 RX14 added this to the Next milestone Apr 28, 2018
@RX14 RX14 merged commit 319a3c5 into crystal-lang:master Apr 28, 2018
@makenowjust makenowjust deleted the fix/doc/check-git-worktree branch April 28, 2018 13:52
chris-huxtable pushed a commit to chris-huxtable/crystal that referenced this pull request Jun 6, 2018
@makenowjust @chris-huxtable
docs command: don't show error on console when not a git repository (c…
88bfad8 
…rystal-lang#3700)

* Show warning message if cannot detect GitHub repository

* Remove warning message not to detect GitHub/GitLab repo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@straight-shoota straight-shoota straight-shoota left review comments

@Sija Sija Sija left review comments

@RX14 RX14 RX14 approved these changes

@asterite asterite asterite approved these changes

Assignees
No one assigned
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler topic:tools
Projects
None yet
Milestone
0.25.0
Development

Successfully merging this pull request may close these issues.
5 participants
@makenowjust @asterite @Sija @straight-shoota @RX14