Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Expanded range info shown in HTML repr #821
Expanded range info shown in HTML repr #821
Changes from 3 commits
433e917
bcb8a4c
600c7b2
0fa2f85
c4e90dd
d1cfbf8
d7244b5
3836e04
e7c4d0c
9f9830b
af1465d
7eaf157
f67b710
094722c
c8ed056
bf723f6
f0e3a09
a289244
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's super frequent to define a regex for a
String
Parameter and wouldn't expect all Param users to be knowledgeable about regular expressions. I'd suggest either not including'.*'
or being more explicit with e.g.regex(.*)
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason to include it is only that when
allow_None
isTrue
and there's noregex
, the range is shown as| None
, which I think is confusing, given that I read|
as "or", so it's "???? or None" (i.e., "what or None??"). Can you think of a better way to convey "any string" than.*
?''
would be accurate since the empty string is a regex that matches every string, and then it would show'' | None
, if that would look better. Or, sure,regex(.*)
, if you think that's clearer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I agree that most options here aren't very good. Not sure I love it but if there's no regex I'd also be okay with
str | None
ifallow_None
orstr
otherwise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually
str | None
for no regex + allow_None and nothing at all for no regex andallow_None=False
is my preference.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(That's the same reason I went with (-Inf,Inf) for numbers;
| None
looked odd without it. If anyone has a solution that conveys "any valid input for that type, or None" more clearly, happy to use that instead!)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm; looks like @philippjfr 's notes weren't updated on my screen until after I made the above note. I don't mind "str", but I'd like us to use the same approach for number parameters as for string, since it's precisely the same concept: any allowed string, or any allowed number. Instead of (-Inf, Inf), would a number be "num" in this approach, as in "num | None"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, None is a different type, so I suppose we really ought to be putting it into the Type column! I.e. the type isn't really
Integer
, it'sInteger | None
! I think that will eliminate all the awkwardness on the range field from| None
, so I'll change it to do that unless someone strongly objects. Seems like the obvious next step from Philipp's proposal.Assuming we do that, then there is still a question whether to combine the type and range columns. Combining them makes it clear that the constraints apply to the non-None type, not None:
Integer | None
,Integer [0, Inf) | None
,List[Integer] len(0,10) | None
.I can't quite decide which I prefer; any votes?