Skip to content

Add a words terminal to improve performance - #164

Merged
WillemJann merged 2 commits into
spreadsheetlab:masterfrom
jahav:word-terminal
Nov 28, 2022
Merged

WillemJann merged 2 commits into
spreadsheetlab:masterfrom
jahav:word-terminal

Conversation

@jahav

@jahav jahav commented Oct 10, 2022

Copy link
Copy Markdown
Contributor

PR for #163.

@joerivv joerivv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

After performing functional tests I can confirm that the performance has indeed improved massively. Excellent!

Two remarks:

First, not all Unit Tests run successfully:
Screenshot 2022-10-18 at 16 29 23

See error of the first one:

Assert.AreEqual failed. Expected:<Parsed>. Actual:<Error>. 
   at XLParser.Tests.WordsTerminalTests.TestTerminalMatching(String input, Boolean caseSensitive, String[] words, Func`2 checkTerm) in C:\Users\joeri\Documents\GitHub\XLParser\src\XLParser.Tests\WordsTerminalTests.cs:line 49
   at XLParser.Tests.WordsTerminalTests.RecognizesAnyWordFromList() in C:\Users\joeri\Documents\GitHub\XLParser\src\XLParser.Tests\WordsTerminalTests.cs:line 15

Secondly, I would suggest to consistently use curly braces { } for all loops and if-statements, just for readability. ReSharper suggests a few more things but I think they can be ignored :)

@jahav
jahav requested a review from joerivv October 24, 2022 21:31

@WillemJann WillemJann left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the improvements, the code looks good. Together with PR #162 I get a more than 50% performance improvement of the unit test suite on my machine. Good job!

Sorry for the late response to your pull requests. I will try to release a new version of XLParser today, including your improvements.

@WillemJann
WillemJann merged commit e72584c into spreadsheetlab:master Nov 28, 2022
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.

3 participants