Handle nullable types #30
Conversation
After the change to the text-format indent method we no longer indent blank lines. So I had to fix the fixtures to avoir breaking tests
It removes a bug with two lines between the last property added and the next method. But it adds a blank line between the class's opening brace and the first property when adding a property for the first time. Still a good trade I would say, lot less common
Only the test here, the modifications were already done.
Are you only talking about the handling of templates paths ? The there is the issue of the failing tests in the CI, we should merge |
Yes - would rather keep these two PRs seperate, or am I missing something? |
|
Lots of CS related fails - maybe we need to revert the changes to the example files? |
I will do two PR in the
CS ? |
| // We should not have to import it, we are extracting the | ||
| // method into the same file. We should not try to fix an | ||
| // oversight from the user, espacially since he might want | ||
| // to keep his partial qualified name | ||
| $builder->use((string) $variableType); |
There was a problem hiding this comment.
we need to import it -- f.e.
- a variable was retrieved from a method from another namespace
- the variable has a type
- it is now a parameter in the new method in the current namespace
- we need to import the type.
Can we change the comment to reflect that?
We need to fix the whole qualfiied name issue, but it's a separate thing :)
There was a problem hiding this comment.
Sorry for the delay to respond, I needed to take care of some other stuff and I don't have a lot of time left.
I'm not sure to see the case you're talking about.
Here we are extracting a method from a selection.
So the new method will always be created in the same scope: the class.
Meaning that any variable that we will have to provide as parameters to new method will already be imported in the scope.
If it is not, it's the fault of the user and we have no way of knowing which one to import in case there is multiple possibilities.
So I understand that we can try, just in case there is only one possibility.
But I don't think it's really SRP, here we should only build a method from the information at our disposal in the scope.
We should not modify the scope, even to fix an error.
If you still want to use the import I will change the comment for the one you proposed.
There was a problem hiding this comment.
Meaning that any variable that we will have to provide as parameters to new method will already be imported in the scope.
This is not true (I previously fell for this) -- if you call a method on an imported class, that method can return a type not in the current scope and you will therefore need to import it if it's a parameter of the extracted mehtod.
| $parameterBuilder->type($variableType->short()); | ||
| // TODO Create a utils class or something to get the shortest type by taking | ||
| // into account the import table, to handle aliases and partial namespaces | ||
| $shortTypeAsString = ($variableType->isNullable() ? '?' : '') . $variableType->short(); |
There was a problem hiding this comment.
can we remove the TODO. This is to do with the qualified names topic :)
|
this looks great -- can we create a utility class to format the type? or is it makes sense, make it part ofthe |
It's a bit old in my head right now but I think we would need a separate service to achieve that. So I would go for a
What do you think ? |
|
After a year I think I'll close it to cleanup, might even be done in phpactor/code-builder#19 :) |
Go along with phpactor/worse-reflection#65
Fix some tests after the changes and handle types generation for completing the constructor and implementing contracts.