Skip to content

Conversation

notjagan
Copy link

It currently puts the parsed array into other_literals3.

@physikerwelt
Copy link
Member

@notjagan please fix the failing tests

@physikerwelt
Copy link
Member

@HowardCohl @notjagan what is the progess here?

@HowardCohl
Copy link
Member

@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
Copy link
Author

notjagan commented Aug 5, 2016

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.

@@ -0,0 +1,18 @@
0 info it worked if it ends with ok
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the log file, please.

@notjagan notjagan changed the title Create parsing for texvc.sty using texutil.js. Create parsing for texvc.sty using texutil.js Aug 9, 2016
lib/parseSty.js Outdated
}
sections = sections.slice(0, sections.length - 1);
return sections;
return string.match(/\{[^\{\}]*?(([^\}]*)\}+)*?[^\{\}]*?\}|\[[^\[\]]*?(([^\]]*)\]+)*?[^\[\]]*?\]/g).map(function(s) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Germany one calls that "to reinvent the wheel".
How about using a well tested library for that like
https://www.npmjs.com/package/balanced-match
?

lib/parseSty.js Outdated
"\\DeclareRobustCommand",
"\\defSpecFun"];

// Returns a two dimensional array, where each element contains the body of a bracket section and the type of bracket
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@notjagan
Copy link
Author

notjagan commented Aug 15, 2016

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.

lib/parseSty.js Outdated
cmds[commands[0]] = commands[1];
break;
case 1:
var cmd = definition.replace(new RegExp(sections[0][0].replace(/\\/g, "\\\\"), 'g'), "").match(/\\[a-zA-Z]+/)[0];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still not clear to me, what this line does. @HowardCohl do you understand that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would make sense to introduce a a variable for sections[0] ?
Would that be \command1,'{' int the example below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@physikerwelt @notjagan says he fixed this in the latest pull request

@physikerwelt
Copy link
Member

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

@physikerwelt
Copy link
Member

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

test/parseSty.js Outdated
parseSty.newCommand("\\newcommand\\test\\test\\test", "\\newcommand", {});
}, function(err) { return err instanceof Error; });
});
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newline at the end of the file please

.gitignore Outdated
lib/optionalFunctions.csv

# Node log
lib/npm-debug.log
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a newline at the end of the file, please.

@physikerwelt
Copy link
Member

physikerwelt commented Aug 16, 2016

only one more newline and you are done ;-)

@physikerwelt physikerwelt merged commit 42f4db6 into DRMF:master Aug 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants