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

Modernize documentation #618

Merged
merged 16 commits into from
Mar 21, 2020
Merged

Modernize documentation #618

merged 16 commits into from
Mar 21, 2020

Conversation

SzymonKaminski
Copy link
Contributor

This PR aims to modernize the bundle's documentation by resolving several opened issues.

One twig template file still uses code-block html+jinja as html+twig would not work currently as mentioned in #613 (review)

phpcs.xml.dist file from this repository was used when running phpcs in SF skeleton, therefore parameter and return type hints were not added. Additionally @MongoDB annotations spanning multiple lines (instead of one) were left unchanged.

Every issue is resolved in separate commit containing brief description of changes, unless sensible splitting of given line(s) was hard to achieve.

Using doctrine/rst-parser directly in my Symfony skeleton or SenseException/rst-builder produced errors as mentioned in #613 (comment) (e.g. Unknown directive: "tip" for line ".. tip::").
I set up doctrine-website locally and confirmed that docs based on this PR can be built without any errors.

@SenseException
Copy link
Member

html+twig will currently run into a warning.

@malarzm
Copy link
Member

malarzm commented Feb 17, 2020

@SzymonKaminski thank you a lot for your effort and this PR! Just wanted you to know that we need to handle #613 first, then this PR will prolly need a rebase

@malarzm
Copy link
Member

malarzm commented Feb 27, 2020

@SzymonKaminski sorry for the long time, but we've finally merged #613 and merged it up into 4.1.x branch. Could you please rebase onto that branch and also target 4.1.x with this PR? That's current maintained branch so docs changes you want to introduce need to go there. Also I hope that work done in #613 and in #622 won't make too much trouble for you but I wanted to let you start with green build :)

Use https everywhere, replace broken anchor links with working
alternatives. Reword paragraph mentioning DoctrineExtensions
adding links to actual package and bundle documentation.
Avoid words like 'just', 'simply' and making any assumptions
about knowledge of a user. Replace 'easiest' with 'recommended'.
Avoid repetitions of words, use punctuation marks in proper places.
Replace Acme bundles with App. Remove generate:bundle command.
Update file paths to new directory structure. Update installation guide
to cover required steps when not using Flex. Prefer project_dir over root_dir.
Reorder `use` statements, prefer [] over array(). Fix typos. Remove duplicated
DefaultController example as the first one includes autowired DocumentManager
already. Remove dynamic findOneBy methods examples.
UserInterface anchor no longer exists in Symfony docs - linking to Security.
@SzymonKaminski
Copy link
Contributor Author

The lint action is green and I believe I haven't missed anything during the rebase 🎉

this is pretty easy. Add the following method to the ``DefaultController``
of the bundle:
setter methods, you're ready to persist data to MongoDB. Let's try it from inside
a controller. Create new Controller class inside source directory of your project:
Copy link
Member

@malarzm malarzm Feb 28, 2020

Choose a reason for hiding this comment

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

DefaultController was already created in this chapter, please bring back the part about adding a new method to it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the top of the file I've removed section with bin/console generate:bundle --namespace=Acme/StoreBundle command from which DefaultController came from

Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry then 👍 so we need to just change

Suggested change
a controller. Create new Controller class inside source directory of your project:
a controller. Create a new Controller class inside source directory of your project:

Also what do you think about having entire controller in example? Showing only method to add made sense when controller was already there, now it looks a bit off IMO

Copy link
Contributor Author

@SzymonKaminski SzymonKaminski Feb 28, 2020

Choose a reason for hiding this comment

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

Ok, will add class name to the example. Also the missing file path inside codeblock in the next section Fetching Objects from MongoDB. That example doesn't need full class, right?

Copy link
Member

Choose a reason for hiding this comment

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

That example doesn't need full class, right?

Right 👍

Resources/doc/events.rst Outdated Show resolved Hide resolved
Resources/doc/config.rst Outdated Show resolved Hide resolved
Resources/doc/config.rst Outdated Show resolved Hide resolved
@malarzm
Copy link
Member

malarzm commented Mar 21, 2020

On a weekend thought: this is PR is so good I'm gonna change that one thing off after merging, let's not keep this open anymore. Thanks a lot @SzymonKaminski! 🎉

@malarzm malarzm merged commit acc6652 into doctrine:4.1.x Mar 21, 2020
@malarzm malarzm added this to the 4.1.1 milestone Mar 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants