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

Synthesize Github WikiLinks #175

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tweekmonster
Copy link

This makes it a little easier to locally preview wiki pages. WikiLinks syntax is converted to standard Markdown links before submission to the Github API. Additionally, DirectoryReader will append the file extension to filename if it's missing.

@joeyespo joeyespo mentioned this pull request Apr 29, 2016
@joeyespo
Copy link
Owner

joeyespo commented Apr 29, 2016

Thanks for the PR, @tweekmonster!

I'm going to think about this.

On the one hand, this gives Grip the ability to render Wiki-style GFM. On the other, this could cause confusion when using Grip for its intended purpose.

For example, it'd be easy to forget to add the .md extension (or somebody may try using Wiki-style links since that's what they know). Since it renders locally, they'd expect it to render on GitHub too. After pushing, they'll be surprised to see that it doesn't--that is, if they take the time to check it.

For that reason, I'm reluctant to pull in non-GFM behavior like this.

How else could Wiki-specific formatting be handled that wouldn't interfere with normal Grip usage?

@tweekmonster
Copy link
Author

@joeyespo I thought about this too in hindsight. I think adding a --wiki flag would be a clear indication of the user's intent. I didn't have the time to figure out how to add an additional flag, though.

@ezhukovskiy
Copy link

Please merge it asap! This is what I really need to use grip!

@tweekmonster
Copy link
Author

@joeyespo Do you have any thoughts on adding a --wiki flag so that WikiLinks conversion would be a deliberate user action?

It looks like adding a new flag would require changes to a lot of function signatures, unless you're not opposed to having a module variable set if --wiki is found in the cli arguments.

@joeyespo
Copy link
Owner

joeyespo commented Jun 8, 2016

@tweekmonster I'm not a heavy user of the GitHub Wiki pages, so it's hard for me to know if a particular wiki-related feature belongs in grip.

That said, I'm very willing to dive in since Wiki pages are a big part of GitHub. More importantly, the rendering seems to have a lot of overlap with GFM. I'm up for experimenting with a --wiki flag. We could it "beta" to move things forward until it becomes stable enough.

Are you up for adding the --wiki flag to this PR?

@joeyespo
Copy link
Owner

joeyespo commented Jun 8, 2016

@tweekmonster One approach would be to subclass GitHubRenderer with GitHubWikiRenderer and override the render method (and expose wiki_patch there too):

class GitHubWikiRenderer(GitHubRenderer):
    def render(self, text, auth=None):
        if not self.user_content:
            text = self.wiki_patch(text)
        return super(GitHubWikiRenderer, self).render(text, auth)

    def wiki_patch(self, text):
        ...

As for DirectoryReader, it could take a new constructor parameter called infer_extensions (defaulting to False). When true, your addition can be taken into account.

Then finally, add a new wiki parameter to the Grip class, which uses the above (here and here). (Perhaps right after the title parameter.) And finally, adjust api.py to mirror this change.

I think that should cover it. This would open the doors to playing with other wiki-specific features.

@tweekmonster
Copy link
Author

it's hard for me to know if a particular wiki-related feature belongs in grip.

As far as I can tell, WikiLinks is the only unique markup for wikis. My use case for grip was quickly previewing wiki pages locally because textarea editing is annoying. I don't think the footer or side bar rendering are really necessary, but would be nice.

One approach would be to subclass GitHubRenderer with GitHubWikiRenderer

This is pretty helpful! Now I have an idea of where to get started. I was having trouble following the trail. 😅

@tweekmonster
Copy link
Author

@joeyespo I added the --wiki flag. I didn't squash the commit because I wanted you to be able to go over the changes. I'm not sure if I covered everything that would need to know about wiki rendering.

@EugeneZee If you're in a rush to have WikiLinks, you can run: pip install git+https://github.com/tweekmonster/grip.git@wiki-links

@ezhukovskiy
Copy link

@tweekmonster amazing!!! thanks, it works great, but these is one more enhancement that is needed to be done:
[[images/mypicture.png]] should be changed to an image not a reference.

@tweekmonster
Copy link
Author

@EugeneZee I think images will be outside the scope of this PR. Besides, I never needed to use images in GitHub wikis, so I don't think I'm qualified to ensure it would work as expected.

@tweekmonster
Copy link
Author

@EugeneZee Actually, your example image tag is a local image right? It's not relative to a GitHub repo or anything like that? If that's the case, I think it would be trivial to add that.

@ezhukovskiy
Copy link

@tweekmonster Exactly, it is a local image. Github allows to upload images to a sub-folder and then if it sees a link to a local image it converts it to an image tag.

@tweekmonster
Copy link
Author

@joeyespo Have you been able to review the changes?

@tweekmonster
Copy link
Author

Ping @joeyespo

@to01z
Copy link

to01z commented Jan 10, 2020

Is there anything that prevents this from going into master (aside from the resolve)? It would be awesome to be able to use Grip for wiki preview as well.

If there is concern about stability, perhaps the link patch that happens in the renderer could be made conditional on the wiki switch as well. That way there should be no functional change unless the user opts in using the switch.

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

Successfully merging this pull request may close these issues.

4 participants