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 code #10126

Merged
merged 1 commit into from Oct 17, 2022

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Oct 12, 2022

  • migrate to attributes;
  • add helpful phpdoc annotations;
  • use typed properties;
  • add type declarations.

@greg0ire
Copy link
Member Author

🤔 should I be targeting 2.13.x? After all, there is not much reason to wait for this.

private $firstComment;
/** Unidirectional - Many-To-One */
#[ManyToOne(targetEntity: Comment::class)]
private Comment $firstComment;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private Comment $firstComment;
private Comment|null $firstComment = null;

ManyToOne relations are nullable by default.

@derrabus
Copy link
Contributor

🤔 should I be targeting 2.13.x? After all, there is not much reason to wait for this.

I wouldn't wait with the 2.14.0 release too long either, so targeting 2.14.x is just fine imho.

@greg0ire greg0ire changed the base branch from 2.14.x to 2.13.x October 15, 2022 09:55
@greg0ire
Copy link
Member Author

I wouldn't wait with the 2.14.0 release too long either, so targeting 2.14.x is just fine imho.

I retargeted to 2.13.x so as not to have things to convert when merging up.

@greg0ire greg0ire force-pushed the migrate-docs-to-attributes branch 4 times, most recently from 8535f95 to 45d53e4 Compare October 15, 2022 14:03
@greg0ire greg0ire marked this pull request as ready for review October 15, 2022 14:10
@greg0ire greg0ire changed the title Migrate documentation to attributes Modernize documentation code Oct 15, 2022
docs/en/cookbook/working-with-datetime.rst Outdated Show resolved Hide resolved
docs/en/cookbook/working-with-datetime.rst Outdated Show resolved Hide resolved
docs/en/reference/association-mapping.rst Show resolved Hide resolved
docs/en/reference/dql-doctrine-query-language.rst Outdated Show resolved Hide resolved
docs/en/reference/inheritance-mapping.rst Outdated Show resolved Hide resolved
docs/en/reference/transactions-and-concurrency.rst Outdated Show resolved Hide resolved
docs/en/reference/working-with-associations.rst Outdated Show resolved Hide resolved
docs/en/reference/working-with-associations.rst Outdated Show resolved Hide resolved
docs/en/reference/working-with-objects.rst Outdated Show resolved Hide resolved
docs/en/tutorials/composite-primary-keys.rst Outdated Show resolved Hide resolved
@greg0ire greg0ire force-pushed the migrate-docs-to-attributes branch 4 times, most recently from b964d68 to c1eef4a Compare October 15, 2022 17:29
@greg0ire
Copy link
Member Author

@derrabus please take another look 🙏

docs/en/cookbook/decorator-pattern.rst Outdated Show resolved Hide resolved
docs/en/cookbook/decorator-pattern.rst Outdated Show resolved Hide resolved
docs/en/cookbook/decorator-pattern.rst Outdated Show resolved Hide resolved
docs/en/tutorials/composite-primary-keys.rst Outdated Show resolved Hide resolved
docs/en/tutorials/composite-primary-keys.rst Outdated Show resolved Hide resolved
docs/en/tutorials/composite-primary-keys.rst Outdated Show resolved Hide resolved
docs/en/tutorials/composite-primary-keys.rst Outdated Show resolved Hide resolved
docs/en/tutorials/ordered-associations.rst Outdated Show resolved Hide resolved
docs/en/tutorials/ordered-associations.rst Outdated Show resolved Hide resolved
- migrate to attributes;
- add helpful phpdoc annotations;
- use typed properties;
- add type declarations.

Co-authored-by: Alexander M. Turek <me@derrabus.de>
@greg0ire
Copy link
Member Author

Shall we propagate CPP here?

I applied your suggestions and removed the property that was outside the constructor for that particular comment.

@greg0ire greg0ire added this to the 2.13.4 milestone Oct 17, 2022
@greg0ire greg0ire merged commit 5f4b76b into doctrine:2.13.x Oct 17, 2022
@greg0ire greg0ire deleted the migrate-docs-to-attributes branch October 17, 2022 21:11
Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

PHP 7.4 is nearly gone, but it still feels weird to have attributes as the primary configuration when PHP 7 is still supported in composer :-D

@@ -451,17 +449,15 @@ besides specifying the sequence's name:

.. configuration-block::

.. code-block:: php
.. code-block:: attribute
Copy link
Member

Choose a reason for hiding this comment

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

As we have several formats here, shouldn't we keep the annotation code-block too ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be better yeah, I'll try to find some time to do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

See #10157

* @DiscriminatorMap({"cc" = "Test\Component\ConcreteComponent",
"cd" = "Test\Decorator\ConcreteDecorator"})
*/
#[Entity]

Choose a reason for hiding this comment

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

Despite the fact that branch is already merged: I personally prefer multiple attributes in single '#[...]' looks a bit cleaner ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

It was like that before 🤷


public function addPropertyChangedListener(PropertyChangedListener $listener)

private array $_listeners = array();

Choose a reason for hiding this comment

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

Short array?

Copy link
Member Author

Choose a reason for hiding this comment

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

Please consider sending a PR

@@ -32,11 +32,12 @@ upon insert:

class User
{
const STATUS_DISABLED = 0;
const STATUS_ENABLED = 1;
private const STATUS_DISABLED = 0;

Choose a reason for hiding this comment

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

If referring to 8.1, enum example can be used here :)

#[Id, Column(type: 'integer')]
private int|null $id = null;
#[Column(type: 'string')]
private string $name;

Choose a reason for hiding this comment

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

Probably should provide a default value, otherwise: creating new entity and calling getName will return fatal - if getter is strict too.

Copy link
Member Author

Choose a reason for hiding this comment

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

On the other hand, you might have a constructor preventing this entirely.

* @Cache("NONSTRICT_READ_WRITE")
*/
#[Entity]
#[Cache(usage: 'NONSTRICT_READ_WRITE')]

Choose a reason for hiding this comment

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

This string can be a real constant

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean?

return $this->commentsRead;
}

public function setFirstComment(Comment $c) {
/** @param Collection<int, Comment> $c */

Choose a reason for hiding this comment

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

Wrong comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch 👍

private $comments;
private int $id;

/** @var Collection<int, Comment> */

Choose a reason for hiding this comment

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

Here: key can be mixed as we don't know how people are adding to collection via add or set :)

Copy link
Member Author

Choose a reason for hiding this comment

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

And if they do, they should get a warning from PHPStan/Psalm thanks to this annotation, so I stand by it.

@greg0ire
Copy link
Member Author

To anyone finding more stuff to say after the merge: please send PRs instead 🙏

#[DiscriminatorMap(['cc' => Component\ConcreteComponent::class,
'cd' => Decorator\ConcreteDecorator::class])]
Copy link
Contributor

Choose a reason for hiding this comment

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

See discussion here slevomat/coding-standard#1456

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.

None yet

6 participants