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

use mimetype to decide if typesetting is needed #38

Merged
merged 4 commits into from
Aug 5, 2023

Conversation

rnwgnr
Copy link
Contributor

@rnwgnr rnwgnr commented Aug 2, 2023

When the path portion of the URI contains a dot - for example in the query part like in geminispace.info/search?test.com - the page is not handled by typesetgmi. Instead of matching the path part of the URI we check the mimetype from meta instead.

I'm currently unsure if the dot in a query part should be percent-encoded anyway (quick research lead to ambigous results). At least for now astro does not percent-encode it.

When the path portion of the URI contains . - for example in the query part
like in `geminispace.info/search?test.com` - the page is not handled by `typesetgmi`.
Instead of matching the `path` part of the URI we check the mimetype from meta instead.

I'm currently unsure if the dot in a query part should be percent-encoded anyway (quick research
lead to ambigous results). At least for now astro does not percent-encode it.
@blmayer
Copy link
Owner

blmayer commented Aug 2, 2023

Hello, the MEDIA response is optional, can you add the empty case to the switch?

@rnwgnr
Copy link
Contributor Author

rnwgnr commented Aug 3, 2023

oh well, i wasn't aware of this - thanks for the hint.

As i understand the spec mime type is mandatory when serving anything other then text/gemini but can be omited when it is text/gemini.
I'm not sure if charset can be set without giving a mimetype though - we might need to make an assumption here.

I gonna update the PR

@rnwgnr rnwgnr changed the title use mimetype to decide if typesetting is needed DRAFT: use mimetype to decide if typesetting is needed Aug 3, 2023
According to the spec an empty mimetype should be treated as
"text/gemini".
@rnwgnr rnwgnr changed the title DRAFT: use mimetype to decide if typesetting is needed use mimetype to decide if typesetting is needed Aug 4, 2023
astro Outdated
*".gmi"|"") typesetgmi ;;
*.*) cat ;;
*) typesetgmi ;;
case ${meta#[0-9][0-9]" "} in
Copy link
Owner

Choose a reason for hiding this comment

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

Hmmm, this ${#} is specific to bash, we cannot use this, it breaks POSIX standard.

Whats is wrong with meta to need this?

I think line 380 is enough to be
*"text/gemini"* | "") typesetgmi ;;

Am I missing something?

Copy link
Contributor Author

@rnwgnr rnwgnr Aug 4, 2023

Choose a reason for hiding this comment

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

hm - according to https://pubs.opengroup.org/onlinepubs/9699919799/ this behaviour is defined for POSIX shells and shellcheck does not complain about it being undefined in POSIX.

meta contains the status code as well (e.g. 10 text/gemini), "" would not match.

update: i tried in dash and it works fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nevermind - i finally get what you where talking about 🤦

@blmayer
Copy link
Owner

blmayer commented Aug 4, 2023

Hi @rnwgnr, I left a comment on your changes.

@rnwgnr

This comment was marked as outdated.

@rnwgnr
Copy link
Contributor Author

rnwgnr commented Aug 5, 2023

please squash the commits to hide my idiocracy 🤣

@blmayer
Copy link
Owner

blmayer commented Aug 5, 2023

Hahahah there is no idiocracy here. You made a great work. Thanks for your time.

@blmayer blmayer merged commit e9b6580 into blmayer:main Aug 5, 2023
@rnwgnr rnwgnr deleted the typeset branch August 5, 2023 16:39
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.

2 participants