Sanitize whitespace by os #255
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #255 +/- ##
==========================================
+ Coverage 89.80% 89.81% +0.01%
==========================================
Files 26 26
Lines 2874 2877 +3
Branches 505 506 +1
==========================================
+ Hits 2581 2584 +3
Misses 157 157
Partials 136 136 ☔ View full report in Codecov by Sentry. |
ivojawer
commented
Jun 24, 2024
| @@ -103,6 +103,8 @@ const comment = (position: 'start' | 'end' | 'inner') => lazy('comment', () => r | |||
| const sameLineComment: Parser<Annotation> = comment('end') | |||
|
|
|||
| export const sanitizeWhitespaces = (originalFrom: SourceIndex, originalTo: SourceIndex, input: string): [SourceIndex, SourceIndex] => { | |||
| const EOL = input.includes('\r\n') ? '\r\n' : '\n' | |||
There was a problem hiding this comment.
Aca tenia varias opciones para conseguir el valor de EOL
- Se podria sacar del objeto
osque provee Node, pero no me parecia correcto porque no sabemos con certeza si el string que llega fue generado en el mismo SO que esta corriendo wollok-ts.- Aca fallaban los tests en Windows porque los strings de js van siempre con
\nme parece, pero se podria mockear supongo.
- Aca fallaban los tests en Windows porque los strings de js van siempre con
- Calcular una vez para todos los archivos que se quieran parsear, pero tambien me hacia ruido y ademas no se que tanto mas performante seria.
Termine dejandolo asi pero no estoy seguro.
There was a problem hiding this comment.
Yo creo que por ahora es una solución good enough para salir. Por la performance no me preocuparía mucho, no veo que vaya a afectar.
👍🏼
fdodino
approved these changes
Jun 24, 2024
| @@ -103,6 +103,8 @@ const comment = (position: 'start' | 'end' | 'inner') => lazy('comment', () => r | |||
| const sameLineComment: Parser<Annotation> = comment('end') | |||
|
|
|||
| export const sanitizeWhitespaces = (originalFrom: SourceIndex, originalTo: SourceIndex, input: string): [SourceIndex, SourceIndex] => { | |||
| const EOL = input.includes('\r\n') ? '\r\n' : '\n' | |||
There was a problem hiding this comment.
Yo creo que por ahora es una solución good enough para salir. Por la performance no me preocuparía mucho, no veo que vaya a afectar.
👍🏼
PalumboN
approved these changes
Jun 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
En Windows los source maps se estaban formando mal, esto es porque en los sistemas operativos
de bienbasados en Unix el caracter de new line es el\npero en Windows son dos caracters\r\nAhora el/los caracter/es a utilizar son calculados y el calculo para sanitizar los sourcemaps se basan es esto.