DRMF / texvcjs

A LaTeX validator/translator for TeX strings embedded in wikitext
0 stars 2 forks source link

Create parsing for texvc.sty using texutil.js #46

Closed notjagan closed 8 years ago

notjagan commented 8 years ago

It currently puts the parsed array into other_literals3.

physikerwelt commented 8 years ago

@notjagan please fix the failing tests

physikerwelt commented 8 years ago

@HowardCohl @notjagan what is the progess here?

HowardCohl commented 8 years ago

@physikerwelt It turns out that this was a pull request made by Claude, not @notjagan . Also, it has to do with the texvcjs repo, @notjagan is working on the texer repo.

notjagan commented 8 years ago

While I was getting the code to pass the tests, I noticed two possible inconsistencies between texvc.sty and the preexisting test cases. The first is that the test cases look for \or to be replaced with \lor, whereas texvc.sty defines the replacement as \orMediaWiki for \lor. Since the texvc.sty definition only appears once and the test cases had \or twice, I changed texvc.sty so it defined the replacement for \or. Additionally, in texvc.sty, the replacement for \varcopper was defined as \mbox{\\coppa}, but the double backslash didn't make sense and wasn't even recognized by the pegJS, so I removed the second backslash from texvc.sty. If either of these changes were incorrect, please notify me and I can change them back.

physikerwelt commented 8 years ago

I went through the pull request again. It looks much better now. However, there are still some suggestions I have. Especially the lib/parseSty.js is still hard to understand. It would be good to write the code in a way that you, other students and me are able to improve it in the future afer not having looked at the code for a while.

physikerwelt commented 8 years ago

@notjagan please create tests for each method in parseSty.js Moreover, try to make your code more readable. To do that there are two options. Either simplify the code or add comments.

notjagan commented 8 years ago

@physikerwelt I am adverse to using if (!sections) or anything similar because this just checks if sections is falsy. Although this is not really a concern with arrays such as sections in my code, other types like integers do encounter issues here, so for consistencies sake I think I should stick to using if (typeof sections === "undefined") or switch to if (sections == null), which is the standard method of checking if a variable is null or undefined. The reason I was not already using if (sections == null) is because I wasn't sure if jslint would give me issues because it doesn't use ===.

physikerwelt commented 8 years ago

@notjagan I know. But using ! is more common and easier to read. I started with the undefined method as well but thereafter I was advised by more experienced developers that ! should be used instead.

notjagan commented 8 years ago

I wrote tests for a majority of the methods within parseSty.js. However, there are two methods (parseLine and parseNewCommand) I haven't written test cases for because they are heavily (and exclusively) used by another parent function (parseSty), and the parent function already has test cases written for it. As such, they are already tested by the test cases for the parent function. Additionally, the functions do not return anything of use and only do in-place modification of passed arrays, so test cases in the current style would not be fitting for the checks required to test them independently.

physikerwelt commented 8 years ago

Well done! The comments help to understand your code. However, there are some things that still are not clear to me.

physikerwelt commented 8 years ago

@notjagan each function should have at least one test case. But you can create different ways of testing your methods.

physikerwelt commented 8 years ago

only one more newline and you are done ;-)