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.

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

Jump to bottom

Fix camelCase duplicated properties generator. #881

Merged
merged 3 commits into from
Mar 10, 2020
Merged

Fix camelCase duplicated properties generator. #881

merged 3 commits into from
Mar 10, 2020

Conversation

bop10
Copy link
Contributor

@bop10 bop10 commented Jan 20, 2020

Fixes #880

@bop10 bop10 mentioned this pull request Jan 20, 2020
Closed
@mfn
Copy link
Collaborator

mfn commented Jan 20, 2020

@bop10 tests are failing, see the red ones at https://travis-ci.org/barryvdh/laravel-ide-helper/builds/639458535?utm_source=github_status&utm_medium=notification

@bop10
Copy link
Contributor Author

bop10 commented Jan 20, 2020 •
edited
Loading

@mfn I reverted my changes in local, runned phpunit, and the same tests fail (it seems that it's not related to my changes).
Then, I fresh cloned the repository, and the same tests still fails.

Update: Looks like the $actualContent variable is always empty (that's why the 3 tests fail, ResetAndSmartReset test folder)

@mfn
Copy link
Collaborator

mfn commented Jan 20, 2020

@bop10 bummer; thanks => #882 will fix that!

@mfn
Copy link
Collaborator

mfn commented Jan 20, 2020

Btw, your fix is correct AFAICS, thanks 👍

Small wish: could you add a test which, in general, works with model_camel_case_properties and shows this off?

That would be awesome (copypasta of existing one should solve 80% of the work)

@bop10
Copy link
Contributor Author

bop10 commented Jan 21, 2020

Ok! I'll try and update this pull request

@bop10
Copy link
Contributor Author

bop10 commented Jan 21, 2020

Tests added and passed 👍

mfn
mfn approved these changes Jan 21, 2020
View reviewed changes
Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

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

Thank you 🙇

@mfn
Copy link
Collaborator

mfn commented Feb 3, 2020

I fixed some other tests which failed, can you rebase please? I hope the checks are green afterwards. Thank you!

@bop10
Merge pull request #1 from barryvdh/master
e59a3b2 
tests: fix after recent merged "stepping on each others toes" (barryvdh#882)
@bop10
Copy link
Contributor Author

bop10 commented Feb 3, 2020

Done 👍

@mfn
Copy link
Collaborator

mfn commented Feb 3, 2020

@barryvdh IMHO we're good here!

@davidGM-00
Copy link

Hey, is there an estimated time to merge this one? Thanks!

@mfn
Copy link
Collaborator

mfn commented Feb 25, 2020

Try requesting a review from @barryvdh if you can

@bop10
Copy link
Contributor Author

bop10 commented Mar 10, 2020

@barryvdh Can you take a look at this one? Thanks!

@barryvdh barryvdh merged commit 9043701 into barryvdh:master Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfn mfn mfn approved these changes

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

Successfully merging this pull request may close these issues.

Camel case properties is not working as expected
4 participants
@bop10 @mfn @davidGM-00 @barryvdh