Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(Parsers): Re-use old parser names to new parser, closes #622 #549 #628

Merged
merged 2 commits into from
Dec 19, 2019

Conversation

ocehugo
Copy link
Contributor

@ocehugo ocehugo commented Dec 18, 2019

This is a requirement for transparent usage of the
new Staroddi Parser that supports Mini v0, Mini v1, DST Tilt
and DST CTD.

The reason why is done this way is to still support
old insturments.txt configuration files, database
key:value associations, and to be transparent to the user.

This is a requirement for transparent usage of the
new Staroddi Parser that supports Mini v0, Mini v1, DST Tilt
and DST CTD.

The reason why is done this way is to still support
old insturments.txt configuration files, database
key:value associations, and to be transparent to the user.
@ocehugo ocehugo requested a review from lbesnard December 18, 2019 00:40
@ocehugo ocehugo changed the title feat(Parsers): Re-use old parser names to new parser feat(Parsers): Re-use old parser names to new parser, closes #622 #549 Dec 18, 2019
return
catch

end
Copy link
Contributor

Choose a reason for hiding this comment

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

is this really the best way to do this? bypassing the entire code? couldn't this be done in the function calling this parser?
otherwise, it would be great to add some comment above the try catch about the reasons why all the rest of the code is bypassed. And maybe a printf or something in after the catch to mention the default parser is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this really the best way to do this? bypassing the entire code? couldn't this be done in the function calling this parser?

It's the most transparent way for the end-users. Users and their configurations (toolbox and db's) are sometimes hardcoded for the older parser names. Moving to a different name would not allow backward compatibility - I discussed yesterday with gg.

t would be great to add some comment above the try catch about the reasons why all the rest of the code is bypassed. And maybe a printf or something in after the catch to mention the default parser is used?

Yes, I'll put a msg on the exception. I was thinking that the user should not be aware of the fail but if we don't print anything we will never know if someone had a problem with the new parser.

The right way is to completely remove the entire code outside the try and just fail miserably. Although we got a good test coverage for this parser and the old parser is pretty much deprecated, I'm just fencing a bit high here until q3 release.

@ocehugo
Copy link
Contributor Author

ocehugo commented Dec 19, 2019

@lbesnard done

@ocehugo ocehugo force-pushed the Staroddi_parser_workaround branch from 86166dc to e950bbb Compare December 19, 2019 02:29
@lbesnard lbesnard merged commit 4fbf41c into master Dec 19, 2019
@lbesnard lbesnard deleted the Staroddi_parser_workaround branch December 19, 2019 02:30
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.

2 participants