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. 👍 |
|
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.
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.
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.
You can remove these comments now the tests have been added.
There was a problem hiding this comment.
Good catch, just did it.
|
I added unit tests for this PR, courtesy of @Zamralik for helping me transpile them into Deno compliant tests. |
Hey!
I made a few changes to this file to make it simpler and avoid common pitfalls.
I removed repeated calls to
charCodeby simply defining constants at the beginning of the file.charCodebecame unused so I removed it.strcould be simplified to one line using a simple ternary so I did it.readLinecould also be simplified so it became a ternary.skipSpacegot improved readability and does not repeatedly callscharCode. Comparison has also been factorized.readLineSlicewas entirely rewrote to avoidwhile(true). It made it shorter and more comfortable to read and understand. It's purpose should be clearer. I letskipSpaceexist 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
readLineSliceprovides a similar behavior.I did not include those tests to the pull request. I can indeed include them if necessary.
Cheers!