Closed jwoertink closed 7 years ago
Overall this looks good to me. I haven't found edge cases with assignments.
However, Crystal allows declaring constants not only with all capitals and underscores (which surprised me 😄):
Constant = 10
Constant # => 10
Constant = 20 # => already initialized constant Constant
A22 = 22
A22 # => 22
and this does not work in icr...
icr(0.23.1) > Constant = 10
dynamic constant assignment
Constant = 10
^
icr(0.23.1) > A22 = 22
dynamic constant assignment
A22 = 22
^
Ah good catch! I forgot about those. I updated the spec to catch for that sort of thing as well.
Thanks for the fix!
I found another interesting case:
icr(0.23.1) > A = 1
=> ok
icr(0.23.1) > A == 1
=> true #=> is not a constant_assignment, good
icr(0.23.1) > A =~ /test/
=> ok #=> is a constant_assignment, bad
Should be easily fixed by changing the regex:
^[A-Z]+([a-z_0-9].+)?\s*\={1}[^=]
to
^[A-Z]+([a-z_0-9].+)?\s*\={1}[^=~]
haha! Another good catch. I didn't realize how much that =
is used lol. Ok, so this adds a catch for the regex and also a negative spec to make sure we still see dynamic constant assignment when necessary.
Looks good to me. @greyblake your turn 😄
Looks good to me!) Can be merged :)
On Sep 14, 2017 20:19, "V. Elenhaupt" notifications@github.com wrote:
Looks good to me. @greyblake https://github.com/greyblake your turn 😄
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/crystal-community/icr/pull/59#issuecomment-329566769, or mute the thread https://github.com/notifications/unsubscribe-auth/AAG7aNknqarCO0tm5sclRLrCHVdjX3-Iks5siW4XgaJpZM4PW8wn .
Good job. Thanks, guys 👏
This PR catches assignments to constants like
But it will still ignore constant usages like
IO::Memory.new
orString.build
. This will also ignore when checking equality between 2 constants likeA == B
.