Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Array redeclare fix #333

Merged
merged 4 commits into from
Apr 19, 2022
Merged

Array redeclare fix #333

merged 4 commits into from
Apr 19, 2022

Conversation

j3j5
Copy link
Contributor

@j3j5 j3j5 commented Mar 18, 2022

Fixes #253

@j3j5
Copy link
Contributor Author

j3j5 commented Mar 18, 2022

Not completely happy with the str_replace, any other ideas are welcome!

@j3j5
Copy link
Contributor Author

j3j5 commented Mar 21, 2022

Oooops, I'll try to fix the tests later today.

dbrekelmans
dbrekelmans reviewed Mar 21, 2022
View reviewed changes
generator/src/PhpStanFunctions/PhpStanType.php Outdated
@@ -89,6 +89,11 @@ public function getDocBlockType(?int $errorType = null): string
} elseif ($this->nullable && $errorType !== Method::NULLSY_TYPE) {
$returnTypes[] = 'null';
}

if (in_array('array', $returnTypes)) {
$returnTypes = array_map(fn (string $type) => str_replace('array', '\array', $type), $returnTypes);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, now that I see this, I remember that I've also had this issue with DateTime. Do you think there's a (scalable) way to do this for all PHP namespace types/objects (keeping https://wiki.php.net/rfc/namespaces_in_bundled_extensions in mind)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, interesting, didn't realize DateTime also has a function file and a class file. Another solution would be to change the filenames of the generated functions. Say, if we use array_func.php and datetime_func.php (for example, whatever that doesn't match the original return types) there won't be any issue when the autoloader looks for the types. That actually may be more "scalable".

It is debatable (at least, I couldn't find any authoritative info on it) whether the primitive types on phpdoc shouldn't be the first option unless explicitly defined otherwise, so this very well may be a bug on PHPDocParser, but it just seemed easier to fix here.

@Kharhamel
Copy link
Collaborator

This is great. Do you need think you can fix the CI yourself or do you need help with the tests?

@j3j5
Copy link
Contributor Author

j3j5 commented Mar 22, 2022

@Kharhamel Yeah, it shouldn't be a problem, I just need to find the time to do it, haha, but I think I can pick it up some time this week for sure.

@Kharhamel
Copy link
Collaborator

This is great. Don't hesitate to ask questions if you need

@j3j5
Copy link
Contributor Author

j3j5 commented Mar 30, 2022

I've fixed the tests here. I've tried rebasing with master but apparently the generator is broken there atm, it throws an error that I should delete some line from the custom file, didn't want to do that on this PR. Let me know when master is working again and I'll rebase this branch against it.

@Kharhamel
Copy link
Collaborator

I just updated the generator

@j3j5 j3j5 force-pushed the array-redeclare-fix branch from 504d4bf to ba48d3a Compare March 30, 2022 12:30
@j3j5
Copy link
Contributor Author

j3j5 commented Mar 30, 2022

I've rebased the branch and now phpunit is reporting no errors.
Regarding the conversation in here, it got stalled but the more I think about it the more I think that it may be an easier (and better) solution to just use a different name for the files to avoid conflicts with the autoloader. I'm going to try and create a different PR with that solution so you can choose which one you think is better. I should have it ready today eod.

@j3j5
Copy link
Contributor Author

j3j5 commented Mar 31, 2022 •
edited
Loading

Didn't realize the latest changes from master on the generator brought new return types for array and the \ isn't being added on them. I'll try to look into it later, but the more I think about it the better the file renaming solution looks.

@j3j5 j3j5 mentioned this pull request Mar 31, 2022
Closed
@Kharhamel
Copy link
Collaborator

I talked with @moufmouf and we are going to to go with this solution rather than #338 . I just need you to fix the ci (you just need to update the doc)

@j3j5
Copy link
Contributor Author

j3j5 commented Apr 5, 2022

I'll try to get to this today.

@Kharhamel
Copy link
Collaborator

I am sorry, I still cannot merge. I don't know why i am not notified when you answer.

@j3j5 j3j5 force-pushed the array-redeclare-fix branch from 4a91860 to 5e3e492 Compare April 13, 2022 22:07
@j3j5 j3j5 force-pushed the array-redeclare-fix branch from 5e3e492 to 25c39b0 Compare April 13, 2022 22:08
@j3j5
Copy link
Contributor Author

j3j5 commented Apr 13, 2022

Ok, sorry it took me this long, I've rebased against master and regenerated the files with the latest docs. Please double check but I think it's looking fine now.

@Kharhamel
Copy link
Collaborator

Great. I am deploying this right now

@Kharhamel Kharhamel merged commit 3be3115 into thecodingmachine:master Apr 19, 2022
@Kharhamel
Copy link
Collaborator

https://github.com/thecodingmachine/safe/releases/tag/v2.2

@snapshotpl snapshotpl mentioned this pull request Apr 19, 2022
Closed
Kharhamel added a commit that referenced this pull request Apr 19, 2022
@Kharhamel
Revert "Merge pull request #333 from j3j5/array-redeclare-fix"
a09f5cf 
This reverts commit 3be3115, reversing
changes made to 9373d4c.
Kharhamel added a commit that referenced this pull request Apr 19, 2022
@Kharhamel
Merge pull request #345 from thecodingmachine/revertArrayFix
dc50e64 
Revert "Merge pull request #333 from j3j5/array-redeclare-fix"
@j3j5 j3j5 mentioned this pull request May 2, 2022
Closed
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@dbrekelmans dbrekelmans dbrekelmans left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot redeclare Safe\array_combine()
4 participants
@j3j5 @Kharhamel @dbrekelmans @jfoulquie-tnw