Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
create tao migrations script #2409
create tao migrations script #2409
Changes from 17 commits
a51b8ff
68471d2
047ff90
8324a83
88f72d9
9992fd1
2dfe4e6
cecd995
73ae505
241e744
08c3e67
fc249cd
0b5b17b
31ccc88
3f7a027
625acc5
2ba22af
b6ada6e
04d9feb
bee5709
5865bef
2ce546b
510c1c6
af25ede
bcd4c7e
0ac5c38
3a2257d
35f028c
a7d36b7
c81e977
6bf1bba
346a20f
20ee6f5
7b9c255
06d58f5
1a62139
81d9f86
f65538b
60a555a
a711875
ac32cb9
5a621de
58dda0a
70e59cd
a884298
88d2f0c
4affa0f
58a930e
2db74b5
32171e9
7ac53a3
4a364a0
d08bc8f
be33468
420177c
a9ae090
c894350
cec8f56
2f9e932
7904aa2
a7dc333
9bb7288
29de1dd
500c571
d92a374
c3b0027
f17ff93
859d844
d6d65b5
dbf9731
4d244ae
f8344ea
50bda9c
e96ff96
6ac37b2
024f900
1df19f9
2ec1251
13576af
6c3fa48
57f308b
b3081b4
e24f23d
544f9d0
73e65d3
253c69f
2a891fc
eeac1b1
5d1d21a
d197497
dbeacff
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say that having a more detailed description of the class would be nice for comprehension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command building part could be a good candidate for decoupling. It's being spread all across this class and spoiling its internal interface while introducing other code smells (control flag parameters, magic constants).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the only responsibility of this class is to build command as it is only wrapper of doctrine migration tool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does more than composing migration commands:
ScriptAction
and deals with actual executionAll these to be addressed in this PR is not my attention, rather to raise your awareness as there's lot more here than command-wrapping. These could be tackled step by step later on. The mindset here is to make the code better piece by piece, iteration by iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this actions are done by doctrine migration tool.
Two responsibilities of this class is to configure doctrine migration tool and proxy commands to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Half of these (2,3,4,5,6). And we have an agreement plus I resolve this conversation :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's no other way but a control flag, resolving this magic constant will greatly increase one's understanding on this part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually IDE does the trick:
But i can move it to the constant if you prefer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I see only the value, I have no clue about what it's actually doing :(
And this is the first thing I turn off in my storm :)