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

fix: add support for text/plain; charset=utf-8 #269

Closed
wants to merge 1 commit into from

Conversation

JKRhb
Copy link
Member

@JKRhb JKRhb commented Sep 21, 2021

This PR fixes #33. It adds support for the Content-Format text/plain; charset=utf-8 as an input parameter. The default output Content-Format will now be changed to text/plain; charset=utf-8 so this might actually be a breaking change. I hope, however, that other implementations will be able to handle this.

@JKRhb JKRhb force-pushed the text-plain-fix branch 2 times, most recently from 494c28e to b58e3e7 Compare September 21, 2021 18:33
@JKRhb JKRhb changed the title Use text/plain; charset=utf-8 instead of text/plain while maintaining backwards compatability Add support fortext/plain; charset=utf-8 Sep 21, 2021
@JKRhb JKRhb changed the title Add support fortext/plain; charset=utf-8 Add support for text/plain; charset=utf-8 Sep 21, 2021
@JKRhb JKRhb changed the title Add support for text/plain; charset=utf-8 fix: add support for text/plain; charset=utf-8 Sep 21, 2021
@JKRhb
Copy link
Member Author

JKRhb commented Sep 21, 2021

We might have to find a better way for dealing with content-formats which contain additional parameters in the future. At the moment, only the part before the first ; is being used. This approach, however, is not usable with new content-formats like application/cose; cose-type="cose-encrypt" or application/cose; cose-type="cose-mac" where the parameters actually contain important information.

@JKRhb JKRhb added the bug label Sep 21, 2021
@JKRhb
Copy link
Member Author

JKRhb commented Sep 21, 2021

@awwright: Would you still be interested in this issue?

@awwright
Copy link

I recall the reason I didn't was because it could break backwards compatibility in applications that use merely text/plain to refer to format 0. I'm not sure how much of a problem this actually is or a good way to fix it.

@JKRhb
Copy link
Member Author

JKRhb commented Sep 21, 2021

I recall the reason I didn't was because it could break backwards compatibility in applications that use merely text/plain to refer to format 0. I'm not sure how much of a problem this actually is or a good way to fix it.

Hmm, yeah, I am not so sure either. Maybe a more complex process (i. e. actually parsing the parameters) is needed to distinguish different content-formats, at least when they are given as an input. In any case, considering the potential breaking change it might be better to defer this PR to version 1.0.0.

@coveralls
Copy link

coveralls commented Sep 21, 2021

Pull Request Test Coverage Report for Build 1282106479

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.006%) to 89.828%

Totals Coverage Status
Change from base Build 1280604248: 0.006%
Covered Lines: 1091
Relevant Lines: 1182

💛 - Coveralls

@JKRhb
Copy link
Member Author

JKRhb commented Sep 28, 2021

I now opened and merged #282 which deals (partly) with the parameters issue. I am closing this PR for now but Content-Format 0 should probably be revisited in the future.

@JKRhb JKRhb closed this Sep 28, 2021
@JKRhb JKRhb deleted the text-plain-fix branch September 28, 2021 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Format 0 must be utf-8
3 participants