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

Direct "Edit on ..." links to individual pages #975

Merged
merged 1 commit into from Jun 28, 2016

Conversation

Projects
None yet
5 participants
@lorengordon
Contributor

lorengordon commented Jun 27, 2016

This patch adds support for a configuration option edit_uri that is used to generate a link directly to an individual page in the source repository.

Fixes #269

@lorengordon

This comment has been minimized.

Contributor

lorengordon commented Jun 27, 2016

I had a need for this feature and didn't see further progress on it since the earlier PRs were closed, so I took a stab at it. I did study #752 and #802, and read through the original issue #269. Hopefully I addressed the concerns and this aligns with the desired approach. Let me know if you'd like me to adjust anything.

@@ -143,6 +146,7 @@ def get_page_context(page, content, toc, meta, config):
'meta': meta,
'canonical_url': page.canonical_url,
'edit_url': page.edit_url,

This comment has been minimized.

@waylan

waylan Jun 27, 2016

Member

This is deprecated and will be removed in the next release, so lets not even add it here. As the edit_url is page specific, then it should only ever be accessible from the page object as page.edit_url.

{% if repo_url %}
{% if repo_name == 'GitHub' %}
<a href="{{ repo_url }}" class="icon icon-github"> Edit on GitHub</a>
{% if edit_url %}

This comment has been minimized.

@waylan

waylan Jun 27, 2016

Member

This should be {% if page and page.edit_url %}. As mentioned in another comment, edit_url is page specific. I'm okay with no "edit " link when not on a (markdown generated) "page" as there is nothing to edit anyway.

@waylan

This comment has been minimized.

Member

waylan commented Jun 27, 2016

@lorengordon overall this looks pretty good. I made a few inline comments. Also, I don't see any changes to the mkdocs theme. Make those changes and I think this will be good.

@waylan

This comment has been minimized.

Member

waylan commented Jun 27, 2016

Oh, I almost forgot, the new template context variable needs to be documented here. Just add an entry for page.edit_url.

@lorengordon

This comment has been minimized.

Contributor

lorengordon commented Jun 27, 2016

@waylan, I'm on it. This is my first time contributing to mkdocs... Any preference for amending the prior commit vs adding a second? I didn't see any particular guidance in the contributing doc.

@waylan

This comment has been minimized.

Member

waylan commented Jun 27, 2016

We prefer a clean commit history, so if you can amend the previous commit and do a --force push, that would be preferred, but if not we can work with that as well.

@lorengordon

This comment has been minimized.

Contributor

lorengordon commented Jun 27, 2016

👍 Yep, my preference as well. It's not everyone's, so I like to check first.

@lorengordon lorengordon force-pushed the lorengordon:issue-269 branch from 348b587 to 0815e0d Jun 27, 2016

@lorengordon

This comment has been minimized.

Contributor

lorengordon commented Jun 27, 2016

@waylan, ok, that should address the comments. Anything else?

@@ -118,6 +118,7 @@ favicon | [site_favicon] |
page_description | [site_description] |
repo_url | [repo_url] |
repo_name | [repo_name] |
edit_uri | [edit_uri] |

This comment has been minimized.

@waylan

waylan Jun 27, 2016

Member

Let's remove this. We don't actually have a global edit_uri variable. You have to access it through the config as config.edit_uri and I don't think we need or want to advertise that.

This comment has been minimized.

@lorengordon

lorengordon Jun 27, 2016

Contributor

Oh my bad, I misunderstood what that table was for.

This comment has been minimized.

@waylan

waylan Jun 27, 2016

Member

Not a problem. That will be getting a refactor soon, which should clear things up a bit.

<a href="{{ repo_url }}" class="icon icon-bitbucket"> Edit on BitBucket</a>
<a href="{{ page.edit_url }}" class="icon icon-bitbucket"> Edit on {{ repo_name }}</a>
{% else %}
<a href="{{ page.edit_url }}"> Edit on {{ repo_name }}</a>

This comment has been minimized.

@waylan

waylan Jun 27, 2016

Member

I feel like this template could be simplified a little. The link is repeated three times with the same variables. The only difference is the use of class to set the icon for the link. Maybe:

<a href="{{ page.edit_url }}"
    {%- if repo_name == GitHub %}
        class="icon icon-github" 
    {%- elif repo_name == 'Bitbucket' %}
        class="icon icon-bitbucket"
    {% endif %}> Edit on {{ repo_name }}</a>

This comment has been minimized.

@lorengordon

lorengordon Jun 27, 2016

Contributor

I saw that, but was keeping the template changes to a minimum. Let me fiddle with it a bit...

This comment has been minimized.

@waylan

waylan Jun 27, 2016

Member

keeping the template changes to a minimum

I figured as much. Not a big deal, but with the current changes, if makes sense to clean it up now.

@lorengordon lorengordon force-pushed the lorengordon:issue-269 branch from 0815e0d to 73d7b59 Jun 27, 2016

self.edit_url = utils.urljoin(
repo_url+edit_uri,
self.input_path.lstrip('/'))

This comment has been minimized.

@waylan

waylan Jun 27, 2016

Member

Just took a closer look as this method and I have few concerns:

  1. You are using utils.urljoin to join the parts, so you shouldn't need to also be adding / manually.
  2. I don't see were the page specific path is being added here. Shouldn't self.input_path be appended to the end of each url (see this comment)?

This comment has been minimized.

@lorengordon

lorengordon Jun 27, 2016

Contributor
  1. I was cribbing from set_canonical_url(). I can remove those bits if that is not necessary. It gets a little tricky because utils.urljoin() only joins a pair of strings, not a list. i.e. this no worky:

    self.edit_url = utils.urljoin(repo_url, edit_uri, self.input_path.lstrip('/'))
    

    So I have to call urljoin twice, or make sure that repo_url and edit_url are properly constructed so I can concatenate them.

  2. Line 200 adds self.input_path... I am actually building my docs locally to prove to myself that the patch works...

@lorengordon

This comment has been minimized.

Contributor

lorengordon commented Jun 27, 2016

@waylan, further comments addressed.

@lorengordon lorengordon force-pushed the lorengordon:issue-269 branch from 73d7b59 to 0a02a1b Jun 27, 2016

@lorengordon

This comment has been minimized.

Contributor

lorengordon commented Jun 27, 2016

Alright, think the last line comment is now covered.

@waylan

This comment has been minimized.

Member

waylan commented Jun 27, 2016

Thanks. This looks good to me.

The only concern is that this does add a new config setting. I think its necessary to support the feature, but I would like an okay from @d0ugal before merging it. If he's not okay with that, then we would have to hardcode the edit_uri value in for each supported repo. Of course, the current patch gives us support for most any hosting service out-of-the-box, whereas hardcoding the edit_uri would limit support only to the hosting services we code for.

Although, it occurs to me that we could hardcode some defaults for the new setting (if repo_name is github, default to this or if repo_name is bitbucket default to that). That way, users would only need to set the value if they want to use a non-standard URL (a different branch than master) on a known service, or an unknown (to us) service (such as GitLab).

For completeness, I should also note that now that we have #947, its possible for users to easily override the template and put anything they want in the "edit this page link" via the repo block. While template customization is certainly more difficult for users than altering a setting, it doesn't require us to offer support for any specific feature and gives the users much more power to do whatever they want. I would think this is the strongest argument against adding a new setting for this feature.

However, as we specifically advertise this project as a tool for documenting your coding projects, explicitly offering this feature with a config setting is a reasonable expectation for users to have.

@lorengordon

This comment has been minimized.

Contributor

lorengordon commented Jun 27, 2016

Yes, one goal of mine was to support arbitrary repo hosts and branches. I personally like to use a develop branch, and would prefer to send users there rather than whatever branch the repo host likes to "default" to.

I did take note of the comment on #752 about minimizing the number of config options, which is why I combined everything into one setting. Plus, I think it is more flexible this way, anyway.

It appears the config_option classes are able to lookup the config dictionary, so it looks trivial to add a new class, EditUri(), similar to the approach for RepoURL(), that sets defaults for GitHub and Bitbucket.

Or, hmm, don't want to run into ordering issues with the config file. Perhaps just extending RepoURL() would work? I'll twizzle something out...

@lorengordon

This comment has been minimized.

Contributor

lorengordon commented Jun 27, 2016

Also, urljoin() doesn't appear to work that way. Without the trailing slash on its base param, the last fragment gets dropped. So this:

urljoin('https://example.com/foo', 'bar.md')

returns this:

'https://example.com/bar.md'.

https://docs.python.org/3/library/urllib.parse.html#urllib.parse.urljoin

@lorengordon lorengordon force-pushed the lorengordon:issue-269 branch from 0a02a1b to b974126 Jun 27, 2016

@lorengordon

This comment has been minimized.

Contributor

lorengordon commented Jun 27, 2016

Anyway, I added this bit to set default values for GitHub and Bitbucket. Does that seem reasonable?

@lorengordon lorengordon force-pushed the lorengordon:issue-269 branch from b974126 to 2d2ad55 Jun 27, 2016

@lorengordon

This comment has been minimized.

Contributor

lorengordon commented Jun 27, 2016

Going that approach, would it be valuable to add a few unit tests to the RepoURLTest class?

    def test_edit_uri_github(self):

        option = config_options.RepoURL()
        config = {'repo_url': "https://github.com/mkdocs/mkdocs"}
        option.post_validation(config, 'repo_url')
        self.assertEqual(config['edit_uri'], 'blob/master/docs/')

    def test_edit_uri_bitbucket(self):

        option = config_options.RepoURL()
        config = {'repo_url': "https://bitbucket.org/gutworth/six/"}
        option.post_validation(config, 'repo_url')
        self.assertEqual(config['edit_uri'], 'src/default/docs/')

    def test_edit_uri_custom(self):

        option = config_options.RepoURL()
        config = {'repo_url': "https://launchpad.net/python-tuskarclient"}
        option.post_validation(config, 'repo_url')
        self.assertEqual(config.get('edit_uri'), None)
config['edit_uri'] = 'blob/master/docs/'
elif config['repo_name'].lower() == 'bitbucket':
config['edit_uri'] = 'src/default/docs/'

This comment has been minimized.

@waylan

waylan Jun 27, 2016

Member

This is exactly what I had in mind.

@waylan

This comment has been minimized.

Member

waylan commented Jun 27, 2016

Yes, some more tests would be a good idea. Actually, we should probably add a few tests of the new Page.set_edit_url method as well. Just create a few dummy Page objects and confirm the method returns the correct result with various input.

@lorengordon

This comment has been minimized.

Contributor

lorengordon commented Jun 27, 2016

Can you give me a few more pointers for the Page tests? I looked at nav_tests.py and poked around a few others, but it wasn't clear how that part of the code was being tested.

Direct edit links to individual pages
This patch adds support for a configuration option `edit_uri` that
is used to generate a link directly to an individual page in the
source repository.

Fixes #269

@d0ugal d0ugal added this to the 0.16 milestone Jun 27, 2016

@lorengordon lorengordon force-pushed the lorengordon:issue-269 branch from 2d2ad55 to ac5a8a7 Jun 27, 2016

@waylan

This comment has been minimized.

Member

waylan commented Jun 27, 2016

Sigh.

It appears that Page objects are not being tested directly anywhere. Of course, they used to only be simple objects as part of the nav. However, today they are the container for each actual page and are a major part of how things work. Looks like we need to add some tests. If you want to leave that to be addressed separately, that's fine.

@lorengordon

This comment has been minimized.

Contributor

lorengordon commented Jun 27, 2016

Yeah, if you don't mind, I'm not yet confident enough in the mkdocs code and data structures to know how to setup the tests without an example to guide me. I'm happy to add them later once the basic constructs are in place though! Just ping me whenever.

@waylan

This comment has been minimized.

Member

waylan commented Jun 27, 2016

@d0ugal as far am I'm concerned work on this is done. If your okay with the new setting say the word and I'll merge (or you can merge it yourself).

@d0ugal

This comment has been minimized.

Member

d0ugal commented Jun 28, 2016

Sorry for being so quiet until now. You have both done a wonderful job - I was reading along and this was a fantastic example of how to collaborate in a code review. I just tried it out locally and it works well.

I think we are just missing an addition in the release nodes. We can add that later.

Thanks both!

@d0ugal d0ugal merged commit e440cb2 into mkdocs:master Jun 28, 2016

3 checks passed

codecov/project 93.14% (target 90.00%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@viktorbenei

This comment has been minimized.

viktorbenei commented Jun 28, 2016

Sorry if this is not the right channel, just have a related question: is there a reason why you prefer the /blob/ URL (GitHub) instead of the /edit URL? E.g. https://github.com/mkdocs/mkdocs/blob/master/setup.py vs https://github.com/mkdocs/mkdocs/edit/master/setup.py

Based on our experience, users who are not familiar with GitHub (but might want to fix a typo etc.) can have a hard time finding the "edit" option on GitHub, so linking the /edit/ URL is easier for them.

Edit: also the property/config option is called edit_uri, which, for me, would suggest a link to the edit URL, and not to the "view" URL.

Edit2: and of course thanks for everyone who worked on this feature, I just wanted to start working on it when I saw that the PR was merged & the related issue was closed, this is really a "must have" for our use case. So, again, thank you all! 🎉

@marcelstoer

This comment has been minimized.

marcelstoer commented Jun 28, 2016

@lorengordon @waylan fantastic, thank you so much!

so linking the /edit/ URL is easier

I second that, particularly because the link label says "edit" rather than "read"/"view"/etc.

We prefer a clean commit history, so if you can amend the previous commit and do a --force push

@waylan I fully agree but in my own projects I stopped pestering PR authors with that ever since GitHub gave us the option to squash (and edit commit message) on merge.

@d0ugal

This comment has been minimized.

Member

d0ugal commented Jun 28, 2016

Sorry if this is not the right channel, just have a related question

@viktorbenei I think this is fine, the only risk is that it get's lost in the noise. So if the discussion lasts a while then we should go back to the original issue.

I had wondered about using the edit url, my only concern is that if you are not logged in, you can't even view the page. I know, if you are going to edit on GitHub you'd expect users to be logged in, but we are talking about less experienced users, so I think it could be jarring for them.

ReadTheDocs uses the blob url, this can be seen in the pip docs. I wonder if they have had a discussion we can learn from, or it might just be they added it before GitHub had that feature.

I could be persuaded either way, so let's wait for @lorengordon and @waylan's input.

@marcelstoer

This comment has been minimized.

marcelstoer commented Jun 28, 2016

if you are not logged in, you can't even view the page

Riiight...haven't thought of that because I'm never logged out 😉

@d0ugal

This comment has been minimized.

Member

d0ugal commented Jun 28, 2016

Me neither, I had to open a page in incognito! 😄

@viktorbenei

This comment has been minimized.

viktorbenei commented Jun 28, 2016

I didn't know about the non logged in issue either.

The login IMO wouldn't be an issue (you clicked on an edit button after all), but I just found out that if you click the "edit/pencil" icon on GitHub it auto creates a fork for you in the background and shows the edit page automatically, while if you link directly to the URL and you don't have a fork of the repo / haven't clicked the edit/pencil icon before on that repo, GitHub won't show the edit page right away.

Instead it'll present an info text and button to create the fork like this:

screen shot 2016-06-28 at 12 05 34

(This prompt is only shown if you don't have a fork of the project yet, if you clicked the edit/pencil button at least once then you'll get the edit page directly)

Although I still think that an "edit" link should link to the edit page (in general), I'm less sure about it now. WDYT?

@viktorbenei

This comment has been minimized.

viktorbenei commented Jun 28, 2016

That said, this might still be a more guided approach for the user which will eventually lead to the page where they can edit the content directly, instead of knowing that they can click the little pencil icon on the page.

I saw the confusion with developers too, that they don't know about the edit option on GitHub's web UI, and that they think they have to fork & git clone the repository, because the pencil icon is too hidden on the UI.

@d0ugal

This comment has been minimized.

Member

d0ugal commented Jun 28, 2016

Given that this can be changed in your mkdocs.yml (To switch to edit, you would add: edit_uri: edit/master/docs/), I don't think it really matters. We should just pick one and note the alternative in the documentation.

I'm still not sure what the most sensible default is, but I think I am moving towards changing it to the edit page.

@viktorbenei

This comment has been minimized.

viktorbenei commented Jun 28, 2016

I agree, as long as there's an option to select the one which fits the project better I'm fine with either as a default :)

@lorengordon

This comment has been minimized.

Contributor

lorengordon commented Jun 28, 2016

I'm fine with changing the GitHub default to edit. I didn't know about that previously. Doesn't look like Bitbucket has a similar url...appears the edit function is handled in javascript or something? Anyone know otherwise? Here's an example... https://bitbucket.org/jespern/django-piston/src/default/AUTHORS.txt

@lorengordon lorengordon deleted the lorengordon:issue-269 branch Jun 28, 2016

@lorengordon

This comment has been minimized.

Contributor

lorengordon commented Jun 28, 2016

See PR #978 for the change to use the edit URL.

@waylan

This comment has been minimized.

Member

waylan commented Jun 28, 2016

Given that this can be changed in your mkdocs.yml (To switch to edit, you would add: edit_uri: edit/master/docs/), I don't think it really matters. We should just pick one and note the alternative in the documentation.

This was my thought exactly. I'm indifferent as which is the default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment