Add support for different encodings #210

ghaith · 2021-07-24T06:42:27Z

Closes #203

Add an '--encoding' flag to the CLI options to allow setting a custom
encoding.

When no encoding is passed, attempt to read the encoding from BOM
Tread as UTF-8 if all else fails.
This uses the encoding_rs_io crate to do this

codecov · 2021-07-24T06:50:17Z

Codecov Report

Merging #210 (63b07dc) into master (862f5e7) will increase coverage by 0.34%.
The diff coverage is 97.04%.

@@            Coverage Diff             @@
##           master     #210      +/-   ##
==========================================
+ Coverage   95.86%   96.20%   +0.34%     
==========================================
  Files          54       55       +1     
  Lines       21044    21180     +136     
==========================================
+ Hits        20174    20377     +203     
+ Misses        870      803      -67

Impacted Files	Coverage Δ
src/compile_error.rs	`22.22% <0.00%> (-1.59%)`	⬇️
src/cli.rs	`95.70% <100.00%> (+0.53%)`	⬆️
src/lib.rs	`86.17% <100.00%> (+45.94%)`	⬆️
tests/correctness/external_functions.rs	`100.00% <100.00%> (ø)`
tests/integration/external_files.rs	`100.00% <100.00%> (ø)`
tests/tests.rs	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 862f5e7...63b07dc. Read the comment docs.

Add an '--encoding' flag to the CLI options to allow setting a custom encoding. When no encoding is passed, attempt to read the encoding from BOM Tread as UTF-8 if all else fails. This uses the encoding_rs_io crate to do this

riederm · 2021-07-24T07:25:21Z

src/lib.rs

+where
+    T: Read,
+{
+    fn load_source(&mut self, encoding: Option<&'static Encoding>) -> Result<SourceCode, String> {


why mut? this looks strange to me

Has to do with supporting Read traits. I'm trying to get rid of it again. We might need to re-think the SourceContainer strategy here.

if you have a different idea for the SourceContainer I'm very open ... they did not turn out ultra-elegant :-)

riederm · 2021-07-24T08:39:18Z

src/lib.rs


 /// SourceContainers offer source-code to be compiled via the load_source function.
 /// Furthermore it offers a location-String used when reporting diagnostics.
 pub trait SourceContainer {
    /// loads and returns the SourceEntry that contains the SourceCode and the path it was loaded from
-    fn load_source(&self) -> Result<SourceCode, String>;
+    fn load_source(&mut self, encoding: Option<&'static Encoding>) -> Result<SourceCode, String>;


Can you explain why this suddenly gets mut when we introduce encoding?

read() expects a mutable reader, so if we want to implement for a "Read" trait, we need it to be mutable.
I'll try to look into another strategy here.

That said, the only reason I support Read here is for the tests. So I could probably move this into integration tests that load files instead.

riederm · 2021-07-24T08:40:57Z

src/lib.rs

+            .map_err(|err| format!("{:}", err))?;
+        Ok(SourceCode {
+            source: buffer,
+            path: "local".into(),


Is this only used in tests? This may cause confusion if it ever appears in the production mode

Yes, but I wanted to have the actual reading of the strings shared with productive code, so I added "local" for this one. I am not a fan honestly.

Remove the dyn trait as it's not required in this case

riederm

good change, 3 small requests

riederm · 2021-07-26T19:05:00Z

src/lib.rs

-pub type Sources<'a> = [&'a dyn SourceContainer];
+pub trait SourceLocation {
+    /// returns the location of this source-container. Used when reporting diagnostics.
+    fn get_location(&self) -> Option<&str>;


as discussed, lets force a location.
at the moment we don't see a situation where no location-string would be better than a dummy location string (e.g. "<in_memory>" or "<unit_test>".)

Traits are merged back, and we now have a method to get a String out of the reader.

riederm · 2021-07-26T19:05:33Z

src/lib.rs

-    /// returns the location of this source-container. Used when reporting diagnostics.
-    fn get_location(&self) -> &str;
+    fn load_source(self, encoding: Option<&'static Encoding>) -> Result<SourceCode, String>;
+    // fn get_location(&self) -> &str;


this comment stayed here because you wanted to think about joining the two traits again ;-)

riederm · 2021-07-26T19:08:39Z

src/cli.rs

@@ -255,6 +269,22 @@ mod cli_tests {
        assert_eq!(parameters.output_format_or_default(), super::DEFAULT_FORMAT);
    }

+    #[test]
+    fn encoding_resolution() {


lets add a test that defines the behavior if I ask for an unknown decoding via the cli

ghaith force-pushed the encoding branch from 978ad33 to 9193c4f Compare July 24, 2021 07:09

Add support for different encodings

3df34b2

Add an '--encoding' flag to the CLI options to allow setting a custom encoding. When no encoding is passed, attempt to read the encoding from BOM Tread as UTF-8 if all else fails. This uses the encoding_rs_io crate to do this

ghaith force-pushed the encoding branch from 9193c4f to 3df34b2 Compare July 24, 2021 07:23

riederm reviewed Jul 24, 2021

View reviewed changes

ghaith marked this pull request as draft July 24, 2021 09:54

ghaith added 4 commits July 26, 2021 10:15

Consume self in SourceContainer

e278cd1

Remove the dyn trait as it's not required in this case

Separate the location into a different trait

ac8591c

Fix an endless loop when reading files

91a2f8a

Add integration tests for lib methods

7f6b17f

ghaith force-pushed the encoding branch from b322421 to 7f6b17f Compare July 26, 2021 15:02

riederm requested changes Jul 26, 2021

View reviewed changes

ghaith added 2 commits July 26, 2021 22:05

Use a factory method instead of the Trait for the Reader

fb23d3b

Review Comments, add cli tests, recombine the traits

63b07dc

ghaith marked this pull request as ready for review July 26, 2021 20:57

riederm approved these changes Jul 27, 2021

View reviewed changes

ghaith merged commit 5528671 into master Jul 27, 2021

ghaith deleted the encoding branch July 27, 2021 06:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for different encodings #210

Add support for different encodings #210

ghaith commented Jul 24, 2021

codecov bot commented Jul 24, 2021 •

edited

Loading

riederm Jul 24, 2021

ghaith Jul 24, 2021

riederm Jul 24, 2021

riederm Jul 24, 2021

ghaith Jul 24, 2021

ghaith Jul 24, 2021

riederm Jul 24, 2021

ghaith Jul 24, 2021

riederm left a comment

riederm Jul 26, 2021

ghaith Jul 26, 2021

riederm Jul 26, 2021

ghaith Jul 26, 2021

riederm Jul 26, 2021

ghaith Jul 26, 2021

Add support for different encodings #210

Add support for different encodings #210

Conversation

ghaith commented Jul 24, 2021

codecov bot commented Jul 24, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riederm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 24, 2021 •

edited

Loading