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

Docs ammends for example class #77

Merged
merged 4 commits into from
Dec 30, 2017

Conversation

piotrgradzinski
Copy link
Contributor

Guys,
I've decided to share with you my thoughts on maker-bundle docs over this PR instead of raising the new issue to be more clear.

Current version of Creating your Own Makers consists of one stub class than can be used for creating new maker command. I know that the last paragraph explain everything ("For examples of how to complete this class, see the core maker commands") but at this point for me, the user of this bundle, is not really clear even for easy stuff I will have to check core command to see how methods are filled.

Also I can see the following inconsistencies throughout the class:

  1. use Symfony\Bundle\MakerBundle\Str; and use Symfony\Bundle\MakerBundle\Validator; are not used within this example
  2. name feels for me more as class name than format.
  3. We are missing php skeleton files - why should I create such command without code that it may generate?

I've decided to address those in the following PR.

Looking forward to see your thoughts!

Copy link
Member

@yceruto yceruto left a comment

Choose a reason for hiding this comment

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

Look good to me!

@javiereguiluz
Copy link
Member

Even if your proposal is perfectly fine and correct ... I think we should remove all the code of this section and simply explain that you must implement some interface and look at the code of the existing makers to learn how to create them. We do't have resources to maintain all docs updated and all this could change a lot in the coming months. But let's see what do others think. Thanks!

@weaverryan
Copy link
Member

Yea, I think Javier’s idea makes sense. Creating a maker is more advanced: looking at examples and seeing a well-documented interface is more than enough.

+1 for removing the docs and avoiding maintenance in the future!

@piotrgradzinski
Copy link
Contributor Author

piotrgradzinski commented Dec 4, 2017

@javiereguiluz, @weaverryan - for me the argument for not spend additional time on maintaining the documentation is valid. That's true that after every change within the interface/structure the documentation will have to be updated and it will take time and somebody's attention.

On the other hand, one of the most valuable thing for me in Symfony documentation is the simplicity of creating custom/additional classes. For most of the cases when I had to create something custom the documentation was more than enough. I didn't have to dig into the source code to figure out how to do something. Simple copy/paste of code to certain file/folder and filling the gaps did the work (in more complicated scenarios checking the source code was the solution). Going into this direction within Create you own Makers section we will end up with something like "go and see how it's done in Maker Bundle". I wouldn't like that.

So still, I think the direction we have now is a good one.

On the other hand I volunteer to keep this part up to date :)

@piotrgradzinski
Copy link
Contributor Author

Guys!
I would like to ask you about the decision on this one. I think we should make a decision because the current doc we have may confuse developers a little bit so it would be better to either have example updated or removed.

Thanks!

@javiereguiluz
Copy link
Member

javiereguiluz commented Dec 10, 2017

I'm sorry but I'm still against adding this doc. I know it's a short doc and you have offered yourself to maintain it ... but our current infinite backlog of things related to Symfony Docs tells me we shouldn't add this. We won't change the current stressful situation unless we say "No" repeatedly. I'm sorry!

@piotrgradzinski
Copy link
Contributor Author

piotrgradzinski commented Dec 11, 2017

@javiereguiluz - I do respect your POV.

If so would like me to change this and remove the code example from that section and change this part of doc?

@piotrgradzinski
Copy link
Contributor Author

@javiereguiluz - did some changes, removed example code.

Followed the rule, add one remove two :) Are you happy with those changes?

@piotrgradzinski
Copy link
Contributor Author

Guys, any thoughts on this one?

@javiereguiluz
Copy link
Member

Piotr, thanks for simplifying the docs and thanks for your patience during the review process. I'm really sorry I couldn't merge this earlier. Cheers!

@javiereguiluz javiereguiluz merged commit 14aac7b into symfony:master Dec 30, 2017
javiereguiluz added a commit that referenced this pull request Dec 30, 2017
…luz)

This PR was merged into the 1.0-dev branch.

Discussion
----------

Docs ammends for example class

Guys,
I've decided to share with you my thoughts on maker-bundle docs over this PR instead of raising the new issue to be more clear.

Current version of `Creating your Own Makers` consists of one stub class than can be used for creating new maker command. I know that the last paragraph explain everything ("For examples of how to complete this class, see the core maker commands") but at this point for me, the user of this bundle, is not really clear even for easy stuff I will have to check core command to see how methods are filled.

Also I can see the following inconsistencies throughout the class:
1. `use Symfony\Bundle\MakerBundle\Str;` and `use Symfony\Bundle\MakerBundle\Validator;` are not used within this example
2. `name` feels for me more as class name than format.
3. We are missing php skeleton files - why should I create such command without code that it may generate?

I've decided to address those in the following PR.

Looking forward to see your thoughts!

Commits
-------

14aac7b Fixed minor typos
587f9a8 Removing example code from documentation.
23f48a7 Removing void return type for PHP 7.
c435479 Docs ammends for example class.
@piotrgradzinski
Copy link
Contributor Author

@javiereguiluz - no problem! Happy to help and I'm glad you like the new version! Cheers!

@weaverryan
Copy link
Member

Awesome!

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.

None yet

4 participants