Fix EOL issue with tests #370

Geobert · 2018-01-23T21:09:37Z

Normalizing everything cobalt read to LF and and outputing LF as well.
excerpts_CRLF test has fixtures as CRLF, and target as LF

epage

I wanted to get these comments down before I forgot but I don't recommend working on them yet. Going to start a separate conversation about the goal and approach.

epage · 2018-01-25T02:44:46Z

src/document.rs

@@ -2,6 +2,7 @@ use std::collections::HashMap;
 use std::collections::hash_map::Entry;
 use std::default::Default;
 use std::path::{Path, PathBuf};
+use std::iter::FromIterator;


(arbitrary place)

Fix EOL issue with tests Normalizing everything cobalt read to LF and and outputing LF as well. excerpts_CRLF test has fixtures as CRLF, and target as LF typo

Could you get this to conform to conventional changelist format, including calling out Fixes #241?

I didn't point it out in the other PRs because they were purely internal and unrelated to Issues.

epage · 2018-01-25T02:45:26Z

.gitattributes

@@ -0,0 +1,2 @@
+* text eol=lf
+**/tests/fixtures/*_CRLF/** text eol=crlf


Would it be better to have a separate .gitattributes file right there in the test?

Having it local like that seems like it'd help in terms of moving the code around, etc.

I didn't know we can have multiple .gitattributes, good idea!

epage · 2018-01-25T02:46:42Z

tests/mod.rs

@@ -50,6 +53,8 @@ fn assert_dirs_eq(expected: &Path, actual: &Path) {
            .read_to_string(&mut created)
            .expect("Could not read to string");

+        let created = String::from_iter(normalized(created.chars()));


if .gitattributes is saying lf for everything, and the test should be generating lf based output, why is normalization needed here?

uh, good point, I… don't know, and without, it does not work so it seems I failed here… need more work :-/

I forgot one normalization when reading the layout, it makes the situation better but my test still not pass :-/

Found multiples occurrences of read_file some might need normalization as well, I'll see that later

epage · 2018-01-25T02:54:44Z

.gitattributes

@@ -0,0 +1,2 @@
+* text eol=lf


Forgot to ask, why is this needed? Is it just for the tests? If so, should it just live in in tests and target the targets and fixtures folders?

Also, should we be added an editorconfig file(s) to specify our line ending policies?

This is to make sure that the project is checked out with Linux EOL in source code and test (with exception of the *_CRLF folders) so cargo fmt won't messed up the files being different and having the tests using LF and only some using CRLF to make sure the normalizing is working

Oh right, the problem of aligning rustfmt with git.

This seems less obvious enough that it might deserve a comment in the file.

epage · 2018-01-25T03:11:38Z

tl;dr I'm fine with this going in once we work through the issues

So let me see if I can summarize our long conversation from #241

Issue is about getting tests to work regardless of someone's local line ending setting in git
In the investigation, we found
- cobalt's output could produce mixed line endings if the user uses Windows line endings because some sections are converting or inserting line endings (markdown, our markdown reference code)
- cobalt can miss excerpts if the user uses Windows line endings and the default excerpt separator because it specifies a specific line ending policy

There are a mixture of solutions we can apply

Normalize line endings in what we output
Normalize line endings in our tests (see Normalize EOL before diff #335)
- There was concern that this just masks behavior
Detect the user's line endings and convert the output to match it
Use .gitattributes and .editorconfig to encourage certain behavior within cobalt's tests (doesn't impact users)
Encourage repos with users in different environments to use a non-default excerpt_separator like .
- What percent of clients will have users in different environments?
Change the default newline separator
- The one nice thing about our current one is that it is more "natural"

Traits for us to weigh out in evaluating a solution

Preserving the user's line endings
Consistent line endings
Performance
UX Simplicity
Implementation simplicity

My thoughts over time:

Initially concerned about us unintentionally munging the line endings and thinking that needs to be fixed so we can preserve the user's data as-is
- Can you tell I work in an industry where the number one rule is "don't corrupt data" (people might die)?
- I'm also concerned about the users pushing cobalt in directions we never originally considered and needing precise control over their output
Upon realizing some of it was out of our control (markdown), instead of trying to auto-detect or anything, wondering if we should just leave line endings as-is.
- Most environments accept mixed line endings
- It lets the user opt-out of things messing up their line endings, so they can have precise control over their output
If we are to have any ambition for wide adoption, we need to "just work" which means we can't just tell windows users "Oh, go change the excerpt separator"

So while I'm still concerned about giving advanced users the power to do what they want, I think it is reasonable for this to go in now. In the future, we can either re-evaluate and pull this out or add a config flag for people to opt-out. Neither should be done at this moment though.

Geobert · 2018-01-26T20:02:16Z

The route I choose is to normalize the EOL when reading files. 3 normalizations are needed (see diffs) and it make the tests pass :D

epage

Almost there

epage · 2018-01-26T20:20:31Z

tests/fixtures/excerpts_CRLF/.gitattributes

@@ -0,0 +1,2 @@
+# force tests to keep their Windows EOL
+* text eol=crlf


Should there be a non-root editorconfig here?

we can have multiple editorconfig as well? I'll add it tomorrow :D

epage · 2018-01-26T20:21:25Z

tests/mod.rs

@@ -5,6 +5,7 @@ extern crate cobalt;
 extern crate error_chain;
 extern crate tempdir;
 extern crate walkdir;
+extern crate normalize_line_endings;


epage · 2018-01-26T20:21:41Z

tests/mod.rs

-            .expect("Comparison error");
-        let src_file = Path::new(actual).join(&extra_file);
+        let extra_file = entry.path().strip_prefix(result).expect("Comparison error");
+        let src_file = Path::new(expected).join(&extra_file);


What was the intent of changing these variables around?

I found the original name confusing, I forgot to rollback them, it was for me reasoning, I'll rollback this ^^' sorry

epage · 2018-01-26T20:23:06Z

src/document.rs

@@ -194,6 +196,7 @@ impl Document {
                 -> Result<Document> {
        trace!("Parsing {:?}", rel_path);
        let content = files::read_file(src_path)?;
+        let content = String::from_iter(normalized(content.chars()));


I'm sorry I didn't notice this before but ... what do you think of doing the normalization in read_file?

I could see it being nice that read_file is a general abstraction for loading data throughout cobalt. If there are cases where we shouldn't normalize (like maybe cobalt migrate), then we just don't use it.

… I didn't notice read_file was ours… ^^'

epage · 2018-01-26T21:59:44Z

.editorconfig

@@ -0,0 +1,3 @@
+[*]
+end_of_line = lf
+insert_final_newline = true


This file should specify root

root: special property that should be specified at the top of the file outside of any sections. Set to "true" to stop .editorconfig files search on current file.

http://editorconfig.org/

any nested ones shouldn't.

Geobert · 2018-01-27T15:16:54Z

Once the CI are done, I think this is merge-ready :D

Closes cobalt-org#241 add editorconfig add gitattributes

Geobert force-pushed the fix#241 branch from a4b6e0c to 9119a40 Compare January 23, 2018 21:14

epage requested changes Jan 25, 2018

View reviewed changes

Geobert force-pushed the fix#241 branch 2 times, most recently from 3e0a3f9 to 68d6f60 Compare January 26, 2018 20:01

Geobert force-pushed the fix#241 branch from 37864ea to adef8b5 Compare January 26, 2018 20:10

epage requested changes Jan 26, 2018

View reviewed changes

epage reviewed Jan 26, 2018

View reviewed changes

Geobert force-pushed the fix#241 branch from 9430cf5 to fe9d365 Compare January 27, 2018 15:16

Geobert mentioned this pull request Jan 27, 2018

Rename some refactored fct #373

Merged

Geobert force-pushed the fix#241 branch from 9170218 to 91a5bb4 Compare January 27, 2018 16:09

fix(tests): normalizing EOL management

ef16be0

Closes cobalt-org#241 add editorconfig add gitattributes

Geobert force-pushed the fix#241 branch from fe71307 to ef16be0 Compare January 27, 2018 16:14

epage approved these changes Jan 27, 2018

View reviewed changes

epage merged commit 0aedcba into cobalt-org:master Jan 27, 2018

Geobert deleted the fix#241 branch January 27, 2018 19:17

		@@ -0,0 +1,2 @@
		* text eol=lf
		*/tests/fixtures/_CRLF/** text eol=crlf

		@@ -0,0 +1,2 @@
		# force tests to keep their Windows EOL
		* text eol=crlf

Fix EOL issue with tests #370

Fix EOL issue with tests #370

Uh oh!

Conversation

Geobert commented Jan 23, 2018

Uh oh!

epage left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Geobert Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Geobert Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epage commented Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Geobert commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

epage left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Geobert Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Geobert commented Jan 27, 2018

Uh oh!

Uh oh!

Geobert Jan 25, 2018 •

edited

Loading

Geobert Jan 25, 2018 •

edited

Loading

epage commented Jan 25, 2018 •

edited

Loading

Geobert commented Jan 26, 2018 •

edited

Loading

Geobert Jan 26, 2018 •

edited

Loading