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

Fix viewing HTML contents in the terminal for GitHub Enterprise users #92

Closed
wants to merge 4 commits into from

Conversation

dongweiming
Copy link
Contributor

make gitHub enterprise users can use it correctly

@codecov-io
Copy link

codecov-io commented Nov 5, 2016

Current coverage is 95.10% (diff: 100%)

Merging #92 into master will decrease coverage by 0.07%

@@             master        #92   diff @@
==========================================
  Files            34         34          
  Lines          2094       2083    -11   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           1993       1981    -12   
- Misses          101        102     +1   
  Partials          0          0          

Powered by Codecov. Last update 5a24a9b...8351438

@donnemartin donnemartin changed the title Make gitHub enterprise users can use it correctly Fix gh view command for GitHub Enterprise users Nov 6, 2016
@donnemartin donnemartin changed the title Fix gh view command for GitHub Enterprise users Fix gh view command to view HTML contents in the terminal for GitHub Enterprise users Nov 6, 2016
@donnemartin
Copy link
Owner

Hi @dongweiming, thanks for the PR!

To confirm, this is to fix the gh view command so you can see the HTML contents in the terminal for GitHub Enterprise?

@dongweiming
Copy link
Contributor Author

@donnemartin yes. but this also fix gh issuegh repositorygh user

@donnemartin donnemartin changed the title Fix gh view command to view HTML contents in the terminal for GitHub Enterprise users Fix viewing HTML contents in the terminal for GitHub Enterprise users Nov 6, 2016
@donnemartin
Copy link
Owner

Hi @dongweiming, I took a closer look at the PR, thanks again for submitting it.

With the proposed changes on my test GitHub Enterprise instance, the gh view command results in a url like this:

https://1.2.3.4/https://1.2.3.4/donnemartin/foo

I think we'd need to conditionally add self.gh_url, maybe something like this:

def view(self, index, view_in_browser=False):
...
    if self.gh_url not in url:
        url = self.gh_url + url
    self.web_viewer.view_url(url)

Also, it seems we'll need to send auth info in generate_url_contents, otherwise it seems like we're not able to access GitHub Enterprise pages without it (it asks you to sign in).

Viewing https://1.2.3.4/donnemartin/foo

[Skip to content][1]

[ ![GitHub Enterprise logo][2] ][3]

Sign in to your account

Username or email address  Password [Forgot password?][4]

  * [Help][5]
  * [Support][6]

Something went wrong with that request. Please try again.

You signed in with another tab or window. [Reload][7] to refresh your session. You signed out in another tab or window. [Reload][7] to refresh your session.

Are you interested in updating the PR when you get a chance? Thanks!

@dongweiming
Copy link
Contributor Author

@donnemartin i has fix the view url. In my local env, the auth info is right, like this:

Viewing https://github.intra.douban.com/dongweiming/shire.git

[Skip to content][1]

[ ][2]

[Sign in][3]

This repository

  * [Explore][4]

  * [ Watch ][3] [ 19 ][5]
  * [ Star ][3] [ 20 ][6]
  * [ Fork ][3] [ 82 ][7]

 [dongweiming][8]/[shire][9]

[ Code ][9] [ Issues 17 ][10] [ Pull requests 5 ][11] [ Pulse ][12] [ Graphs ][13]

SHIRE

  * [ 2,361  commits ][14]
  * [ 8  branches ][15]
  * [ 0  releases ][16]
  * [ 92  contributors ][17]

1. [ Python 46.0% ][18]
  2. [ JavaScript 40.9% ][19]
  3. [ CSS 10.2% ][20]
  4. [ ActionScript 1.8% ][21]
:
...

the authenticate decorator will ensure the login status, could you try it again?

donnemartin added a commit that referenced this pull request Nov 12, 2016
Clean up base_url class member and property.  Fix urls sent to webbrowser and web_viewer.  Update unit tests.
donnemartin added a commit that referenced this pull request Nov 12, 2016
* dongweiming-url:
  Followup changes in #92
  Fix viewing HTML contents in the terminal for Enterprise
@donnemartin
Copy link
Owner

@dongweiming, this was merged with 5cf1f93.

Thank you!

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