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 #70 #72

Closed
wants to merge 2 commits into from
Closed

Fix #70 #72

wants to merge 2 commits into from

Conversation

mnapoli
Copy link

@mnapoli mnapoli commented Aug 1, 2020

I have taken #71 which was closed and added a test to cover the question asked in the comments (it seems to work fine).

Hopefully that helps fixing this bug.

cc @andytruong, I built my PR on top of yours and kept your commit.

mnapoli added a commit to mnapoli/externals that referenced this pull request Aug 1, 2020
mnapoli added a commit to mnapoli/externals that referenced this pull request Aug 1, 2020
@sorinsarca
Copy link
Member

Well, it works but with some luck. If you somehow manage to follow all those states you'll see that this code should be added in the id_name state.
Also, since we are changing the ReflectionClosure class we should add tests related to it.

Once I push the changes I'll close this PR.

@sorinsarca sorinsarca closed this Aug 4, 2020
@sorinsarca
Copy link
Member

Here is the commit 45de0e3

@mnapoli
Copy link
Author

mnapoli commented Aug 5, 2020

Thanks!

@mnapoli mnapoli deleted the issue-70 branch August 5, 2020 09:13
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.

3 participants