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

Render JSON and YAML inscriptions as text #1449

Merged
merged 3 commits into from
Feb 2, 2023
Merged

Render JSON and YAML inscriptions as text #1449

merged 3 commits into from
Feb 2, 2023

Conversation

casey
Copy link
Collaborator

@casey casey commented Feb 1, 2023

People are publishing YAML and JSON documents as text/plain. Not sure if they should get special treatment (syntax highlighting?) but at least we should recognize the content types so people can use the right content types.

src/media.rs Outdated
@@ -50,6 +50,8 @@ impl FromStr for Media {
}

const TABLE: &[(&str, Media, &[&str])] = &[
("application/json", Media::Text, &["json"]),
("application/yaml", Media::Text, &["yaml"]),
Copy link
Contributor

Choose a reason for hiding this comment

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

.yml is a sometimes used file extension too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems reasonable to me! Added.

@casey casey requested review from raphjaph and removed request for raphjaph February 2, 2023 04:52
@casey
Copy link
Collaborator Author

casey commented Feb 2, 2023

@windsok How comfortable are you reviewing the code? I've been operating with the following policy for merges:

  • If someone contributes code, I review it and merge it
  • If I write code, @raphjaph reviews it before I merge it

This is code I've written, so I'd like to get another review before merging. This PR is simple, but it's nice to sanity check, and make sure my reasoning for a feature makes sense.

  • Does this solve the problem that's trying to be solved?
  • Are there tests?
  • Is this going to be disruptive somehow, etc.

For this PR, since you've taken a look, do you feel comfortable giving an ack? Ideally you should understand the code and what it's doing, and any implications, and if you don't you can ask me questions, even very simple or basic ones, and I'll explain. No pressure if you're busy or not interested!

Copy link
Contributor

@windsok windsok left a comment

Choose a reason for hiding this comment

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

ACK e286752

I understand this change fairly well as I might have hacked in a illegitimate inscription or two 😉

Rendering these as text makes sense. I guess could add some syntax highlighting in the future but this should let people start inscribing their collection JSON etc with the correct mime type

@casey
Copy link
Collaborator Author

casey commented Feb 2, 2023

Thank you!

Also, everything is tested, so when a PR, like this, has a functional change but not a corresponding new test or change to an existing test, you should be suspicious!

In this case, it's okay. So far, we haven't been testing "data", things where really we would just be testing a constant. In this case, this table feels like data, so although we've tested the functions that access this table, we haven't tested individual table entries, which is why this change doesn't have a test.

@casey casey merged commit 10f8469 into master Feb 2, 2023
@casey casey deleted the yaml-json branch February 2, 2023 05:40
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