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

Indicate UTF-8 instead of letting browsers guess. #15

Conversation

MaulingMonkey
Copy link
Contributor

@MaulingMonkey MaulingMonkey commented Dec 7, 2019

Checklist

  • I have read the Contributor Guide
  • I have read and agree to the Code of Conduct
  • I have added a description of my changes and why I'd like them included in the section below

Description of Changes

Indicate UTF-8 instead of letting browsers guess.

Since I'm using HTML5 style, also indicate HTML5 with <!DOCTYPE html>

Bacterius from Discord confused my local programming channel when UTF-8 encoded copyright symbols (©) found in popular licenses such as rdrand 0.4.0's MIT license were misrendered, gaining an extra accented A (©). But not always!

This turned out to be his browser (Chrome) incorrectly guessing the locale was windows-1252 or similar. Since Rust strings are all supposed to be UTF-8 anyways, I think it's safe to claim the output is UTF-8 as well.

If there do happen to be license files with other encodings, translation 'should' happen when first loaded - or better yet, the files themselves should be converted.

Related Issues

None found

Not Changed

I considered suggesting <html lang="en"> as well, although language autodetection is much less likely to go wrong - and "en" might even be incorrect if there's ever a lot of licenses written in, say, Japanese, in some corners of the rust ecosystem. Plus language misdetection shouldn't cause rendering bugs.

Unrelated Chatter

Nice crate! I've created some proc macros wrapping cargo-about to make license text trivial to embed in command line applications. It's also dual MIT OR Apache-2.0 licensed, so feel free to steal upstream any parts of it you find interesting ;).

Bacterius also suggested maybe adding the ability to approve licenses via Cargo.toml to reduce the amount of file clutter. It does have a [metadata] section that could be used for exactly this kind of thing, although I don't mind one way or the other personally:

https://doc.rust-lang.org/cargo/reference/manifest.html#the-metadata-table-optional
https://docs.rs/cargo_metadata/0.9.1/cargo_metadata/struct.Package.html#structfield.metadata

There's also been some chatter about embedding licenses for the Rust stdlib. I actually did some work on enumerating stdlib crates (when stdlib src is installed/available) for licensing information myself before I realized cargo-about existed - let me know if you'd be interested in me trying to extend cargo-about with some of that code. (No promises I'll make the time thought!)

Since I'm using HTML5 style, also indicate HTML5 with <!DOCTYPE html>

Bacterius from Discord confused my local programming channel when
UTF-8 encoded copyright symbols (©) found in popular licenses such
as rdrand 0.4.0's MIT license were misrendered, gaining an extra
accented A (©).  But not always!

This turned out to be his browser (Chrome) incorrectly guessing
the locale was windows-1252 or similar.  Since Rust strings are
all supposed to be UTF-8 *anyways*, I think it's safe to claim
the output is UTF-8 as well.

If there do happen to be license files with other encodings,
translation 'should' happen when first loaded - or better yet,
the files themselves should be converted.
@MaikKlein MaikKlein requested a review from arirawr December 7, 2019 10:16
@MaikKlein
Copy link
Member

Nice crate! I've created some proc macros wrapping cargo-about to make license text trivial to embed in command line applications. It's also dual MIT OR Apache-2.0 licensed, so feel free to steal upstream any parts of it you find interesting ;).

License information for console apps is also something that we need as well. We might want to also publish a library version of cargo-about.

Nice work :)

Bacterius also suggested maybe adding the ability to approve licenses via Cargo.toml to reduce the amount of file clutter. It does have a [metadata] section that could be used for exactly this kind of thing, although I don't mind one way or the other personally:

I had exactly the same thoughts, especially because we also use cargo-deny which requires license information as well.

There's also been some chatter about embedding licenses for the Rust stdlib. I actually did some work on enumerating stdlib crates (when stdlib src is installed/available) for licensing information myself before I realized cargo-about existed - let me know if you'd be interested in me trying to extend cargo-about with some of that code. (No promises I'll make the time thought!)

Definitely interested, we use it internally for one of our projects that has over 400 dependencies, so it might just work. Although right now it only works with simple license expressions. Feel free to open an issue if you run into any problems.

@repi
Copy link
Contributor

repi commented Dec 7, 2019

Filed #16 for libstd license detection, that we will need for sure.

Think we can open a separate issue here also about easy way to include licenses in command-line apps, we will need it ourselves for every CLI binary we distribute (for example for our texture-synthesis-cli: EmbarkStudios/texture-synthesis#72) and lies looks interesting and something we should discuss further!

@Jake-Shadle Jake-Shadle self-requested a review December 8, 2019 14:54
Copy link
Member

@Jake-Shadle Jake-Shadle left a comment

Choose a reason for hiding this comment

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

Thanks! For the other discussions in this PR, they can be reopened either in new PRs or new issues.

@Jake-Shadle Jake-Shadle merged commit 142ada9 into EmbarkStudios:master Dec 8, 2019
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.

4 participants