-
Notifications
You must be signed in to change notification settings - Fork 589
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
Improved textproto/mod.ts readability and maintenability #859
Conversation
I fixed the lint issues. It seems that tests are failing for Canary. I am unsure what Canary is in the development life of this application. |
This is an unrelated issue caused by a change in core (ref #860 ) that I'm fixing at the moment. Just for info, you can upgrade to the canary version with |
@wperron thanks for the answer! If you are already working on this issue, I will not look into it to avoid doing the same work twice. I believe my PR is fixed then so I will not make any change to this PR for now. 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got some minor nitpicks but overall seems nice 👍 If you have the test case(s) I would gladly take them as well
I believe I addressed all requested changes. If there is anything else to change, please let me know. |
textproto/test.ts
Outdated
@@ -189,3 +189,66 @@ Deno.test({ | |||
assertEquals(line, input); | |||
}, | |||
}); | |||
|
|||
/* | |||
Test: 1 null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do these work? What are the expected/actual values? I'm assuming these are valuable tests, so I'm interested in having them run on Deno.test
like the rest of the test suite. As per your other comment, I understand you're unfamiliar with Deno's test runner; Feel free to contact me directly by email or on Discord, or ask around on our community server at https://discord.gg/deno
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a comment that I added accidentally, my bad. Basically @Zamralik designed a test to compare the output of the old code to the output of the new code to ensure behavior was the same. I am adapting those test to verify that the output stays the same. This means I will input some determined values and compare the output to a list of expected determined values.
Those take time to write, if it should be part of this PR, then I will need some time to adapt them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove these comments now the tests have been added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, just did it.
I added unit tests for this PR, courtesy of @Zamralik for helping me transpile them into Deno compliant tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks
Hey!
I made a few changes to this file to make it simpler and avoid common pitfalls.
I removed repeated calls to
charCode
by simply defining constants at the beginning of the file.charCode
became unused so I removed it.str
could be simplified to one line using a simple ternary so I did it.readLine
could also be simplified so it became a ternary.skipSpace
got improved readability and does not repeatedly callscharCode
. Comparison has also been factorized.readLineSlice
was entirely rewrote to avoidwhile(true)
. It made it shorter and more comfortable to read and understand. It's purpose should be clearer. I letskipSpace
exist for now and added a more comprehensive comment about it. I intend to review it's use later to avoid such a hack if possible.These changes pass the provided tests but I also wrote my own to make sure
readLineSlice
provides a similar behavior.I did not include those tests to the pull request. I can indeed include them if necessary.
Cheers!