Skip to content
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

Using chained comparator #4353

Merged
merged 1 commit into from
Aug 26, 2023
Merged

Using chained comparator #4353

merged 1 commit into from
Aug 26, 2023

Conversation

habere-et-dispertire
Copy link
Contributor

Does this read better ?

-$/.Int <= 255 && $/.Int >= 0
+0 <= $/.Int <= 255

Does this read better ?

~~~diff
-$/.Int <= 255 && $/.Int >= 0
+0 <= $/.Int <= 255
~~~
@coke
Copy link
Collaborator

coke commented Aug 25, 2023

I think so, yes.

@habere-et-dispertire habere-et-dispertire marked this pull request as ready for review August 25, 2023 23:26
@raiph
Copy link
Contributor

raiph commented Aug 25, 2023

0 <= $/.Int <= 255

I don't know the context, intent of the example, etc, but why not 0 <= $/ <= 255?

Is there some numeric coercion edge case that I'm not taking into account but would occur in the intended context? (eg if $/ was 「255.6」 then the .Int coercion would make a difference because .Int rounds down to 255, so 0 <= $/.Int <= 255 would be True, but 0 <= $/ <= 255 would be False).

@habere-et-dispertire
Copy link
Contributor Author

habere-et-dispertire commented Aug 25, 2023

my regex ipv4-octet { \d ** 1..3 <?{ 0 <= $/ <= 255 }> }

It looks to me like the match could only be a number comprising of 1 to 3 digits and so an implicit cast is safe ?

FWIW if I run the full code snippet, then I get the same result.

The next example negates things and so things get split up:

$o < 0 || $o > 255

So perhaps it would be even more clearer to use :

$/ ~~ 0..255
$/ !~~ 0..255

and make them more self-similar instead ?

@raiph
Copy link
Contributor

raiph commented Aug 26, 2023

Ah. OK. Yeah. But a few things to note:

  • I was assuming the issue here was some code showing an example of chained comparison. If not, then fair enough.

  • Regardless of anything else, at least for parsing an ip octet (or ip address) it should probably be a token rather than a regex. (Rakoons, and thus examples, ought generally use token as the rule kind they default to using, not regex. And if I understand what's intended with this example, a token would fail faster for many invalid inputs, and thus have better performance.)

  • The token (or regex) needs to be carefully anchored. Consider, for example:

my token ipv4-octet { \d ** 1..3 <?{ $/ ~~ 0..255 }> }
say '301' ~~ &ipv4-octet; # 「30」  <-- Uhoh
  • In practice one would likely write a token (or regex) that deals with a whole ip address in one go. Perhaps any example ought follow suit. Then again, it's definitely getting very complicated for a simple example:
my token ipv4 { ^ (\d ** 1..3) ** 4 % '.' <?{ $0.all ~~ 0..255 }> $ }

say '192.168.0.0.' ~~ &ipv4

@habere-et-dispertire
Copy link
Contributor Author

The document is about regexes, so not sure if going into token land is on topic. It feels like non-trivial changes needs liaison with the primary author. I was just making a small suggestion while reading it.

@coke
Copy link
Collaborator

coke commented Aug 26, 2023

I agree just the small change is fine. Are you happy with the PR as is? @raiph can submit a separate PR for the change they are suggesting.

@habere-et-dispertire
Copy link
Contributor Author

Yes, thank you.

@coke coke merged commit 2458e99 into Raku:main Aug 26, 2023
@coke
Copy link
Collaborator

coke commented Aug 26, 2023

Done, thanks for the cleanup.

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.

None yet

3 participants