Skip to content

Conversation

@JoshClose
Copy link
Contributor

No description provided.

@JoshClose
Copy link
Contributor Author

I don't get the failure locally.

image

@JoshClose JoshClose mentioned this pull request Jan 28, 2021
@joelverhagen joelverhagen merged commit 877e784 into joelverhagen:main Jun 12, 2021
@joelverhagen
Copy link
Owner

Works locally for me too. I'll merge and figured out what's up.

@joelverhagen
Copy link
Owner

Okay, it looks like the CI was running on ubuntu-latest and I presume we are both using Windows. Probably line ending issues.

@JoshClose
Copy link
Contributor Author

Does that mean your tests need to specify the line ending? Libraries should be able to to handle any type of newline, but tests should also be consistent across platforms.

@joelverhagen
Copy link
Owner

Does that mean your tests need to specify the line ending?

Yes 😞. I had to make this change. Previously it was using Environment.NewLine which yielded \n on Ubuntu leading to the test failure.

// We use CR LF consistently instead of Environment.NewLine because some libraries (e.g. CommonLibrary.NET)
// does not handled Unix-style LF gracefully.
const string NewLine = "\r\n";
return Encoding.UTF8.GetBytes(string.Join(NewLine, sourceLines) + NewLine);

Libraries should be able to to handle any type of newline, but tests should also be consistent across platforms.

I looked through the CommonLibrary.NET source code on CodePlex and found code detecting newline by explicitly checking for CR LF (\r then \n). This means it does not handle any type of newline.

As a side not the SEA.CommonLibrary.NET package is compiled with Debug configuration. I'm considering forking the source code onto GitHub myself and publishing my own copy with at least Release configuration and perhaps even a minimal change to handle files with just LF.

@JoshClose
Copy link
Contributor Author

Nice. What a good open source citizen.

@joelverhagen
Copy link
Owner

Done.

GitHub | NuGet

LF is supported and it's built with Release configuration.

@JoshClose
Copy link
Contributor Author

Awesome! In your NuGet description you might want to mention that your version supports LF and is in Release config.

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