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

Handle types as Map of objects, auto add use statements, and some utility methods #67

Closed
wants to merge 10 commits into from

Conversation

sidux
Copy link
Contributor

@sidux sidux commented Dec 10, 2019

Resolves : #66

feature: handle types as Map of PhpType instead of string (it will take into account typehints and phpdoc types, merge both if conflict)
feature: auto add use statement if PhpTypeInterface used as type (for methods, properties and parameters type)
feature: add methods to get the objects for traits, interface, parent class
refacto: rename setMethod, setProperty to addMethod, addProperty
fix : fix the getNullable return value is not bool error

Hi, here is the list of features i was talking about in the issue, this is only to test the logic and i don't expect it to be merged as it is, since it needs more coverage and some changes and it depends if you wan't to keep the backward compatibility or not.

Take a look at this and i will be happy to discuss more about it.

feature: handle types as array instead of string
feature: auto add use statement if PhpTypeInterface used as type
feature: add methods to get traits, interface, parent class as objects
feature: guess the full type when parsing
chore: rename setMethod, setProperty to addMethod, addProperty
feature: handle types as array instead of string
feature: auto add use statement if PhpTypeInterface used as type
feature: add methods to get traits, interface, parent class as objects
feature: guess the full type when parsing
chore: rename setMethod, setProperty to addMethod, addProperty
# Conflicts:
#	tests/fixtures/ClassWithTypes.php
@gossi
Copy link
Member

gossi commented Dec 10, 2019

I only fly over the changes. There is a lot of stuff going on, that I want to address properly and I need your help with this. I cannot map code changes to the features you described here. Can you split this PR into smaller ones? At best I'd ask you to make separate PRs, one for each feature + bugfix. I see there might be interdependencies between them, in that case can you figure out a smart way to do that? (Main idea: code for one PR goes with one mentioned feature).

Second, what I can already tell you, the methods: setMethod() and setProperty() will stay as is. That is, because you cannot add two methods or props with the same name 😉 Simply: $cls->setProperty(PhpProperty::create('foo'))->setProperty(PhpProperty::create('foo')) is nonsense. The semantics for those methods names is: Set this one named property ... and by that be careful with naming it properly. Hope the explanation helps.

@gossi
Copy link
Member

gossi commented Apr 13, 2020

I see five different pull requests in here:

  • 3x feature additions
  • 1x refactor
  • 1x fix

Please split your PR into these five. From that massive code changes it is impossible to map the code change to one of the things mentioned here and by that impossible to review.

@sidux
Copy link
Contributor Author

sidux commented May 29, 2020

@gossi sorry i won't have time to work in this PR again, features i added here are pretty opinionated I will keep them only in my fork. So im closing this, thx for your time and sorry for the inconvenience.

@sidux sidux closed this May 29, 2020
@gossi
Copy link
Member

gossi commented May 30, 2020

Ok. Sorry, couldn't give them more of an understanding here. Happy to help out in the future. Maybe to also handle such custom modifications we can think about an extension API, so you can plug-in your changes?

@sidux
Copy link
Contributor Author

sidux commented Jun 20, 2020

It might help but i guess thats not crucial, i managed to get everything i need with some inheritance and some helpers

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.

Features adding suggestion
2 participants