Skip to content

Conversation

@BCSharp
Copy link
Member

@BCSharp BCSharp commented Oct 14, 2020

So in the end I did not implement support for -bbb, -bbbb and the like. They are nowhere documented and I wonder if anybody is using this in the wild. Anyway, since PythonOptions.BytesWarning is an int-backed enum now, it should be fairly easy to add such support in the future, if the need arises.

Copy link
Contributor

@slozier slozier left a comment

Choose a reason for hiding this comment

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

Seems generally fine to me, although I'm not sure about using the Severity enum. Since it's used for other stuff (and it's in the DLR) we can't really change it without breaking things (although I don't see what possible use they would have of -bbb).

I know Severity was also used in in PythonOptions with IndentationInconsistencySeverity, but that usage can be removed (see #982).

@BCSharp
Copy link
Member Author

BCSharp commented Oct 14, 2020

we can't really change it without breaking things

That's even better. My only concern was, since it is in the DLR, that it may be changed at some point e.g. by adding a value between Warning and Error. Therefore I programmed defensively by using a switch expression rather than a simple cast. (Also since the enum does not use explicit values, I don't think casting is appropriate although the values align perfectly).

If we ever want to support -bbb (or e.g. -bb followed by -b) as a distinct case, we could use FatalError, or introduce a separate int just to trace the number of b used (my preference). This will only be useful to set sys.flags.bytes_warning properly in those scenarios. For the rest of the IronPython code, using enum rather than int makes the code more readable.

Or are you suggesting of making a clone of the enum type within IronPython just for this case? This feels like an overkill.

@slozier
Copy link
Contributor

slozier commented Oct 14, 2020

I guess what's really bugging my about it is that it's also used as an input and Severity.FatalError likely doesn't reflect what the flags "3" value would be, or is meaningless in the case that it's never used. For example, what is the expectation if a user does:

Python.CreateEngine(new Dictionary<string, object> {{"BytesWarning", Severity.FatalError}});

@BCSharp
Copy link
Member Author

BCSharp commented Oct 14, 2020

In CPython, flag 3, 4, ..., means the same as flag 2, so FatalError means Error in this case. But if it is bugging you, I will create a dedicated enum.

@slozier
Copy link
Contributor

slozier commented Oct 14, 2020

In CPython, flag 3, 4, ..., means the same as flag 2, so FatalError means Error in this case.

Yes. I guess my question should have been, what is the user's expectation (one that knows nothing about flags and all that stuff and just wants to accomplish the same thing as the command line argument).

It's probably a non-issue and I'm over-thinking things... PR is fine as-is.

@BCSharp BCSharp merged commit 391a240 into IronLanguages:master Oct 15, 2020
@BCSharp BCSharp deleted the cmdline_opt_b branch October 15, 2020 20:19
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