Skip to content

Fix number bases other than 10 #90

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.

Sign up for GitHub

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

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

Fix number bases other than 10 #90

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b2b4a1a
Fix number bases other than 10
exussum12 Mar 3, 2019
cf5c3cc
Merge remote-tracking branch 'origin/master' into HEAD
exussum12 Dec 14, 2021
b67adb1
update PR
exussum12 Dec 14, 2021
069f0a5
Add missing file
exussum12 Dec 14, 2021
2d9fcae
Clean up
exussum12 Dec 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/Visitor/DetectorVisitor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,17 @@ public function enterNode(Node $node): ?int
foreach ($this->option->getExtensions() as $extension) {
$extension->setOption($this->option);
if ($extension->extend($node)) {
if ((
$scalar instanceof LNumber &&
$scalar->getAttribute('kind') != LNumber::KIND_DEC
)) {
$scalar->value = (int) base_convert(
Copy link
Copy Markdown
Owner

@povils povils Mar 4, 2019

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
Copy Markdown
Owner

@povils povils Jun 24, 2019

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.

$scalar->value,
LNumber::KIND_DEC,
$scalar->getAttribute('kind')
);
}

$this->fileReport->addEntry($scalar->getLine(), $scalar->value);

return null;
Expand Down
2 changes: 1 addition & 1 deletion tests/files/test_1.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ class TEST_1

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

@povils povils Mar 4, 2019

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
Copy Markdown
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.

You think it should return 015 not 15 ?

Copy link
Copy Markdown
Owner

@povils povils Mar 5, 2019

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
Copy Markdown
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.

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
Copy Markdown
Collaborator Author

@exussum12 exussum12 Mar 5, 2019 •
edited
Loading

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
Copy Markdown
Collaborator Author

@exussum12 exussum12 Mar 27, 2019

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

}

for ($i = 3; $i <= 10; $i++) {
Expand Down