-
Notifications
You must be signed in to change notification settings - Fork 323
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
Fix type signature for Integer.parse
to get correct widget for radix
#11954
Conversation
@@ -1287,7 +1287,7 @@ type Integer | |||
Parse the text "20220216" into an integer number. | |||
|
|||
Integer.parse "20220216" | |||
parse text:Text (radix=10:Integer) -> Integer ! Number_Parse_Error = integer_parse text radix | |||
parse text:Text (radix:Integer = 10) -> Integer ! Number_Parse_Error = integer_parse text radix |
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.
Isn't this a flaw of the Enso language?
Personally I never know whether to type the former or the latter variant or some other. I try few and whichever compiles I keep. I wasn't aware there could be a difference between r=10:Int
and r:Int=10
!
Does there have to be a difference? CCing @kazcw.
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 think the behaviour is correct.
radix:Integer = 10
is an argument radix
with type ascription Integer
and default value 10
.
radix=10:Integer
is an argument radix
without type ascription, with default value computed by an expression 10:Integer
- so it is the literal constant 10
plus a type-ascription that asserts that that literal is an integer.
The latter is equivalent to radix=my_default_radix
where
my_default_radix =
c = 10
c : Integer
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.
Yes, that's how it behaves at present. However we could also parse radix=10:Integer
as
radix
argument name10
default expression value- and type ascription
Integer
and interpret it as radix:Integer = 10
probably (if the AST isn't against it completely). Possibly @kazcw could even unify these two cases for us, so we don't have to change anything in the TreeToIr
.
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.
We could but I don't know of any language that allows to reorder types and default arguments like this. Won't that make the language unnecessarily complicated?
Even if we change the parser like you suggest, we still have the case of radix=(10:Integer)
. With the parentheses I think we still should treat it as bound to the expression, not the argument, right?
Pull Request Description
Small fix based on issues found during Advent of Code.
Before:
After:
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.