kursjan / petitparser2

A high-performance top-down parser
MIT License
41 stars 19 forks source link

Enable using custom parser in testing #77

Open tukanos opened 1 month ago

tukanos commented 1 month ago

Improves testing workflow where a user has already parser instantiated and it is different than default one which is offered

tukanos commented 1 month ago

This change: https://github.com/kursjan/petitparser2/pull/77/commits/9f4ccd4bd128356710c56cddbd7454f46e3c05ed is the correct one.

Sorry for the confusion. That is what I get when I try to use github via web and cli git...

kursjan commented 1 month ago

lgtm (looks good to me), but what is the use case for this? Can you add some more details? I am curious (and for posterity) I will then approve the push...

tukanos commented 1 month ago

lgtm (looks good to me), but what is the use case for this? Can you add some more details? I am curious (and for posterity) I will then approve the push...

I'm glad that it looks good to you :).

The reason for PR is that I'm testing multiple parser's variations. For example, when I have a default configuration it has the $, as thousands separator and $. as the decimal separator. That is completely different from both of the ISO standards that are defined and used.

I need to replace the parser with my parser instance (in order to pass the tests), which uses different configuration.

kursjan commented 1 month ago

For your use case, I would have

two classes, representing two variants: ParserVariant1, ParserVariant2. The parserClass would return different variant in different tests.

What throws me off a bit is that parser is assigned in setUp:

{ #category : #running }
PP2CompositeNodeTest >> setUp [
    super setUp.
    parser := self parserInstance
]

a) I don't understand why you need method:

PP2CompositeNodeTest >> currentParser [
    ^ parser
]

This can be achieved by calling:

PP2ParserResource current parserAt: self parserClass

b) I also a bit confused about:

PP2CompositeNodeTest >> parserInstance [
    ^ self currentParser ifNil: [ PP2ParserResource current parserAt: self parserClass ]
]

Why would parserInstance be nil? Isn't it set in setup?

I am sorry, if my questions are missing some point. It is a long time I worked on this and I am looking only on the source code in github, so I might miss something important.

Thanks for explaining!

tukanos commented 2 weeks ago

Sorry for the late answer. I have been fighting Monticello and that is hard battle. I think your questions are to the point.

Ah, I have missed that sending PP2ParserResource>>#parserAt: will setup a new parser. I'll have to test it how it works together with my tests.

I have done in the grammar a method which is able to replace decimal and thousands separator on the current grammar:

replaceDecimalAndThousandsSeparator: aDecimalFormat
    "Replacing decimal and thousands separator if differs from the default"

Since I'm writing this on Friday I'm little bit worn out so I'll test it next week. Your solution is far more eloquent so I hope it will work with the tests I have created (don't see reason why not). If that is the case I'll close this PR as it would be pointless.