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 line endings in co19 repository #2242

Closed
sgrekhov opened this issue Aug 29, 2023 · 6 comments
Closed

Fix line endings in co19 repository #2242

sgrekhov opened this issue Aug 29, 2023 · 6 comments
Assignees
Labels
bad-test Report tests in need of updates. When closed, the tests should be considered good

Comments

@sgrekhov
Copy link
Contributor

Some of the files in co19 repository have CRLF line ending. Need to change it to standard LF but add one test with CRLF line ending to check that tools accept it

See #2240 (comment)

@sgrekhov sgrekhov added the bad-test Report tests in need of updates. When closed, the tests should be considered good label Aug 29, 2023
@sgrekhov sgrekhov self-assigned this Aug 29, 2023
@eernstg
Copy link
Member

eernstg commented Aug 29, 2023

The new CRLF test could have all combinations (a line ending in \n, one ending in \r\n, one ending in \n\r, one ending in just \r, just so we know that this doesn't crash the parser ;-).

sgrekhov added a commit to sgrekhov/co19 that referenced this issue Aug 30, 2023
sgrekhov added a commit to sgrekhov/co19 that referenced this issue Aug 30, 2023
@sgrekhov
Copy link
Contributor Author

The new CRLF test could have all combinations (a line ending in \n, one ending in \r\n, one ending in \n\r, one ending in just \r, just so we know that this doesn't crash the parser ;-).

All tests in this repository now have LF line ending. Added two tests that check that CRLF (Windows) and CR (Old Mac) line endings are supported. As for LFCR (\n\r) it's something non-standard. Is it used somewhere? Do we really need to support it?

athomas pushed a commit that referenced this issue Aug 30, 2023
@eernstg
Copy link
Member

eernstg commented Aug 30, 2023

LFCR (\n\r) it's something non-standard. Is it used somewhere? Do we really need to support it?

I think we can potentially have any and all combinations of line ending characters (for example, because a file gets created on Linux and edited on Windows and then on an old Mac, etc), and the grammar says that they are all possible.

For example, LINE_BREAK derives both \n and \r, so we can have a location in the source code where there is an arbitrary string of \n and \r characters in any order. We don't have to worry about whether that's 1000 lines or 514 lines, because it's never significant for Dart whether we have one new line or multiple (we only rely on whitespace for disambiguation of declarations like @foo (int, int) f() => throw 0;, and the parser should be able to handle it no matter how many lines there are).

So it could still be useful to have that check: "weird combinations of line endings in the same file will not crash the parser". ;-)

@eernstg
Copy link
Member

eernstg commented Aug 30, 2023

Just in case someone at some point in the future runs dos2unix on everything, is there a way to ensure that this very small set of files where it matters can be checked against a known value (perhaps on submit):

> sha1sum line_endings_cr_t01.dart
e5f6d574fd33f3aaf7ba24701d9f8472ba4cd175  line_endings_cr_t01.dart

sgrekhov added a commit to sgrekhov/co19 that referenced this issue Aug 31, 2023
@sgrekhov
Copy link
Contributor Author

Just in case someone at some point in the future runs dos2unix on everything, is there a way to ensure that this very small set of files where it matters can be checked against a known value (perhaps on submit):

> sha1sum line_endings_cr_t01.dart
e5f6d574fd33f3aaf7ba24701d9f8472ba4cd175  line_endings_cr_t01.dart

I'm sure that it's possible but I don't see an immediate solution. As far as I can see SDK tests below also don't have such protection

tests/language/string/multiline_newline_cr.dart
tests/language/string/multiline_newline_crlf.dart
tests/language/string/multiline_newline_lf.dart
tests/language_2/string/multiline_newline_cr.dart
tests/language_2/string/multiline_newline_crlf.dart 
tests/language_2/string/multiline_newline_lf.dart 
tests/lib_2/mirrors/method_mirror_source_line_ending_cr.dart
tests/lib_2/mirrors/method_mirror_source_line_ending_crlf.dart
tests/lib_2/mirrors/method_mirror_source_line_ending_lf.dart
tests/lib_2/mirrors/method_mirror_source_line_ending_test.dart
tests/lib/mirrors/method_mirror_source_line_ending_cr.dart
tests/lib/mirrors/method_mirror_source_line_ending_crlf.dart
tests/lib/mirrors/method_mirror_source_line_ending_lf.dart

Probably it makes sense to create SDK infra issue. But there's a contra as well. This protection will complicate process of changing these tests

@eernstg
Copy link
Member

eernstg commented Aug 31, 2023

Makes sense! I'm trying out the idea in dart-lang/sdk#53398.

@sgrekhov sgrekhov closed this as completed Sep 1, 2023
copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Sep 4, 2023
2023-09-04 sgrekhov22@gmail.com dart-lang/co19#2145. Fix roll failure (LateInitializationError) (dart-lang/co19#2257)
2023-09-04 sgrekhov22@gmail.com dart-lang/co19#2145. Add more local variable declaration tests (dart-lang/co19#2254)
2023-09-01 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 3.5.3 to 3.6.0 (dart-lang/co19#2256)
2023-09-01 sgrekhov22@gmail.com dart-lang/co19#2213. Fix stack trace comparison (dart-lang/co19#2255)
2023-09-01 sgrekhov22@gmail.com dart-lang/co19#2145. Update variables tests according to the current spec. Part 2 (dart-lang/co19#2240)
2023-08-31 sgrekhov22@gmail.com dart-lang/co19#2242. Add LFCR line ending test (dart-lang/co19#2251)
2023-08-31 sgrekhov22@gmail.com dart-lang/co19#2145. Rename Local Variable declaration tests. Delete duplicates (dart-lang/co19#2252)
2023-08-30 sgrekhov22@gmail.com dart-lang/co19#2242. Add line endings tests (dart-lang/co19#2250)
2023-08-30 sgrekhov22@gmail.com Fixes dart-lang/co19#2245. Fix expectations in static_analysis_extension_types_A24_t02.dart (dart-lang/co19#2248)
2023-08-30 sgrekhov22@gmail.com dart-lang/co19#2242. Change CRLF line endings to LF (dart-lang/co19#2249)
2023-08-30 sgrekhov22@gmail.com Fixes dart-lang/co19#2244. Fix typo in dynamic_semantics_member_invocation_A01_t02.dart (dart-lang/co19#2247)
2023-08-30 sgrekhov22@gmail.com Fixes dart-lang/co19#2243. Remove duplicated constructors (dart-lang/co19#2246)
2023-08-29 sgrekhov22@gmail.com dart-lang/co19#2145. Update assertions and descriptions in the current Local variable declaration tests (dart-lang/co19#2241)
2023-08-25 sgrekhov22@gmail.com Fixes dart-lang/co19#2235. Add missing operator tests (dart-lang/co19#2239)
2023-08-25 sgrekhov22@gmail.com dart-lang/co19#2119. Code format and description update in operator tests (dart-lang/co19#2238)

Change-Id: I5aeac05c1aa4cbaa0762ce4f23c1ee916d1006a1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/324040
Reviewed-by: Alexander Thomas <athom@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bad-test Report tests in need of updates. When closed, the tests should be considered good
Projects
None yet
Development

No branches or pull requests

2 participants