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

Add support for short URLs #77

Closed
wants to merge 2 commits into from
Closed

Add support for short URLs #77

wants to merge 2 commits into from

Conversation

oleksis
Copy link

@oleksis oleksis commented Aug 26, 2023

Ex: Twitter short URL
py -m frogmouth https://t.co/5AU9qe7pQ1

image

@davep
Copy link
Collaborator

davep commented Aug 26, 2023

Could you please modify not to change the version number, and provide details of the problem being solved and how it's solving it.

@oleksis
Copy link
Author

oleksis commented Aug 26, 2023

  • Current behavior

The current version of 'frogmouth' v0.9.0 if you pass a short URL like those generated on Twitter, 'frogmouth' opens the default browser with the final URL

  • Expected behavior

This PR adds the behavior that if a short URL is passed follows the redirect and continues with the validation that the "path" is a file with extension Markdown, resulting the Markdown content being rendered in the Viewer widget

@@ -34,8 +37,20 @@ def _(resource: str) -> bool:


@maybe_markdown.register
def _(resource: URL) -> bool:
return maybe_markdown(resource.path)
async def _(resource: URL) -> bool:
Copy link
Collaborator

Choose a reason for hiding this comment

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

While I can see why this would be desirable (even if it's somewhat niche), I'm not sure this is the way go to about it. This suddenly makes maybe_markdown into a single dispatch method that is non-async in some cases but async in another (pyright, for example, quite right complains about this). Also, this suddenly turns the check for a possible Markdown file on the end of a URL into a much more expensive test.

I think there's some merit in handling this case, but I don't think this is the way to go about this.

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.

None yet

2 participants