Added the ability to install an alternate tokenizer (original PR from my main branch)

[brucemack]

Great library! I've added the ability to replace the tokenizer function. I'm using this in an application where I need tokens with spaces (quote delimited). Hopefully this is a reasonable change. Thanks.

[philj404]

I am glad you found this library useful!

This change looks promising. Even though it does add a (very) little complexity, it is nicely separated out in a way that will not distract most users, or add much overhead.

1. Could you add an example (maybe in examples/QuotedSpaceArguments) showing how to add a custom parser? This makes it more obvious to developers why the flexibility can be useful, and how to use it correctly.
2. It would be helpful to have a unit test to ensure future changes do not break (regress) this new feature. Could you add a unit test (some name like examples/tests/CustomParserTest) to confirm that changing the parser uses the new parser correctly?

And thanks for remembering to update the README!

[brucemack]

Excellent, thanks for considering my change. I will take care of the examples/unit tests today. I’m new to the concept of host-based unit tests for Arduino projects so I wanted to make sure you were open to changes before coming up to speed. But unit tests are always a good idea so I’ll create some. Bruce

…

On Dec 28, 2021, at 11:50 PM, Phil Jansen ***@***.***> wrote: I am glad you found this library useful! This change looks promising. Even though it does add a (very) little complexity, it is nicely separated out in a way that will not distract most users, or add much overhead. Could you add an example (maybe in examples/QuotedSpaceArguments) showing how to add a custom parser? This makes it more obvious to developers why the flexibility can be useful, and how to use it correctly. It would be helpful to have a unit test to ensure future changes do not break (regress) this new feature. Could you add a unit test (some name like examples/tests/CustomParserTest) to confirm that changing the parser uses the new parser correctly? And thanks for remembering to update the README! — Reply to this email directly, view it on GitHub <#12 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABLWJNDKVOWFSWCYKF4PMK3UTKHQJANCNFSM5K4ZCMRQ>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.

[brucemack]

When I take Brian Park's latest code (now renamed EpoxyDuino from UnixHostDuino) the SimpeSerialShell unit test are failing. I've traced this to a coincidental change that Brian made in the last few weeks relate to the treatment of line-ending characters in the Print::println() function. For some reason he's changed his mock of the Print class to generate \n instead of \r\n. I don't think this is a good move and I've left him a comment on the commit. I'll see what he comes back with. Meanwhile, I'm going to revert to an earlier version of his library and continue.

I now understand the structure of the unit tests - very cool.

[brucemack]

Unit test, example, and some doc updates committed.

[philj404]

It has been a while since I added the unit tests, so I may be missing something. Your pull request successfully passed my legacy unit tests yesterday, without the need to change to a new line ending.

Should Brian Parks' upstream changes have caused a failure in my tests?

(Perhaps the old "UnixHostDuino" link doesn't pick up newer changes.)

[philj404]

Thank you for updating this dependency to match the new name. I had forgotten about it.

Also, thanks for updating the documentation. It really helps when revisiting the code six months later.

[bxparks]

https://github.com/bxparks/UnixHostDuino redirects to https://github.com/bxparks/EpoxyDuino, so the old link will still work, but it's better to update to the new link.

[brucemack]

Interesting. As you can see, I’m no expert in this space. :-) When I get his latest code (EpoxyDuino) and run the unit tests they fail. When I use the previous version of his code they pass. I wonder if this is related to the fact that he renamed the project from UnixHostDuino->EpoxyDuino. Perhaps the old UnixHostDuino Github project is still accessible in a now-frozen state? Anyhow, I don’t think you should change your line-ending characters - they look right to me. I have created an issue on Brian’s project to try to get some insight into what caused him to change the EOL characters. The change was made ~3 weeks ago so it’s very recent activity. Bruce

…

On Dec 29, 2021, at 2:57 PM, Phil Jansen ***@***.***> wrote: It has been a while since I added the unit tests, so I may be missing something. Your pull request successfully passed my legacy unit tests yesterday, without the need to change to a new line ending. Should Brian Parks' upstream changes have caused a failure in my tests? (Perhaps the old "UnixHostDuino" link doesn't pick up newer changes.) — Reply to this email directly, view it on GitHub <#12 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABLWJNGYNK2HB7ZXXQCMXGDUTNR3TANCNFSM5K4ZCMRQ>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.

[philj404]

Hmm. It looks like the unit tests for this pull request are not running in my environment. I suspect that is because my aunit_tests.yml only run on push. I need to make it run on pull_request too. You are the first contributor that pushed onto a fork. It's hard for me to notice this as an issue, as I have to push to create a pull request. That may be a flaw with EpoxyDuino examples, too (I will follow up).

Since I want to run tests on your code, I want to check out your branch. Your branch's local name -- "main" -- ends up confusing me when I try to pull it into my repository, which has its own idea of "main".

I think I can get it all straightened out, but next time you may want to name your branch to something more obviously related to the change, like brucemack/customTokenizer.

I have plans for the evening. I will experiment tomorrow and let you know what I have learned.

[brucemack]

Hi Phil. I have created a branch called brucemack/customTokenizer in my fork and have submitted another PR from this branch, in case that is easier for you. From my perspective both of the PRs are identical so you should work with the one that works best for you. Please decline the other PR to avoid any confusion. Let me know if I can do anything else to make this easier.

[philj404]

Closing this PR in favor of using #13 . The comments above are still useful though.