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

Fix number bases other than 10 #90

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

exussum12
Copy link
Collaborator

Currently the magic number is detected but the wrong value is shown

This converts the number back to what the user is expecting

eg 015 used to show Magic Number 12

Currently the magic number is detected but the wrong value is shown

This converts the number back to what the user is expecting

eg 015 used to show Magic Number 12
$scalar instanceof LNumber &&
$scalar->getAttribute('kind') != LNumber::KIND_DEC
)) {
$scalar->value = (int) base_convert(
Copy link
Owner

Choose a reason for hiding this comment

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

in this case we are loosing base prefix (0 or 0b). Now dec 10 and binary 10 is displayed in the same way

Copy link
Owner

Choose a reason for hiding this comment

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

i would like more tests regarding this.

@@ -12,7 +12,7 @@ class TEST_1

public function test($input = 4) {
if ($input > 2) {
return 15;
return 015;
Copy link
Owner

Choose a reason for hiding this comment

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

test should fail

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You think it should return 015 not 15 ?

Copy link
Owner

Choose a reason for hiding this comment

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

well if we a respecting base than yes. Also if we want to ignore number 0b10 but not dec 10 would it work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They are 2 separate numbers 0b10 would be ignored if you had 2 in the ignore list currently. so 0x2, 02, 2 and 0b1 would all be ignored by 2.. To get this to return like that we would need to add octal, hex and binary to the cases.

Copy link
Collaborator Author

@exussum12 exussum12 Mar 5, 2019

Choose a reason for hiding this comment

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

Currently this happens for context
image

This PR makes it at least show 755

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You won't be able to tell the difference between 0755 and 0o755 with this

Copy link
Collaborator

@kubawerlos kubawerlos left a comment

Choose a reason for hiding this comment

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

This change definitely needs more tests.

@exussum12 exussum12 added this to the 3.0 milestone Dec 13, 2021
@exussum12 exussum12 requested a review from kubawerlos December 14, 2021 14:39
@exussum12
Copy link
Collaborator Author

@sidz just updated this one too, shows 0755 as 0o755 now rather than 493

@exussum12 exussum12 requested a review from sidz December 16, 2021 21:03
@sidz
Copy link
Collaborator

sidz commented Feb 23, 2022

Hey @kubawerlos.
Could you please take a look on this PR as you requested to do some changes when you have a time?

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.

4 participants