-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
Improve robustness of opts parsing in .opts method #1578
Comments
This syntax isn't heavily used yet as it isn't being used in the documentation. For this reason I think the current approach is good enough and I am re-assigning this issue to the 1.9 milestone. |
As far as I can tell this is just a duplicate of #270, the options parsing doesn't differ between the magic and the |
It does differ: try:
options = OptsSpec.parse(options)
except SyntaxError:
options = OptsSpec.parse(
'{clsname} {options}'.format(clsname=self.__class__.__name__,
options=options)) The first case is identical as you say, the second case is attempted if there is any syntax error. It is a case of 'if it doesn't work for whatever reason, just try something else and hope it works this time'. The assumption is that all syntax errors are because the type wasn't specified - ideally, syntax errors would be more granular and I could only catch the applicable ones but that would be very hard to do. I do think it could be made more robust and that this approach isn't ideal but also that is probably a lot of effort to fix. You can close this issue on that basis if you like... |
Given that we are moving away from the magic syntax and even before the effort/reward balance here was low I think this can be closed. Also #270 is still open, at least for now. |
This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
I want to revisit the new opts string parsing support in the
.opts
method before releasing 1.8 as I think it needs to be made a little more robust. As a reminder, this is now supported:The text was updated successfully, but these errors were encountered: