Skip to content

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented May 30, 2023

Fix PHPLIB-1117

All classes are public API by default, unless marked with @internal.

These classes had none of these 2 annotations @api or @internal and are part of the public api to be consistent with other *Result classes.

  • BulkWriteResult
  • Client
  • Collection
  • Database
  • DeleteResult
  • InsertManyResult
  • InsertOneResult
  • UpdateResult

@GromNaN GromNaN requested a review from jmikola May 30, 2023 11:59
* @see \MongoDB\Collection::watch()
* @see https://mongodb.com/docs/manual/reference/method/db.watch/#mongodb-method-db.watch
*
* @psalm-type ResumeCallable = callable(array|object|null, bool): ChangeStreamIterator
Copy link
Member Author

Choose a reason for hiding this comment

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

CS fix: Incorrect order of annotations groups.

Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

One question about whether we should target function doc blocks as well but overall changes LGTM.

/**
* All classes are public API by default, unless marked with @internal.
*/
$rectorConfig->ruleWithConfiguration(RemoveAnnotationRector::class, ['api']);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think PHPLIB does this, but is there any harm of removing @api from method annotations as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are no @api left.

Copy link
Member

Choose a reason for hiding this comment

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

I'm aware this removed everything that was in PHPLIB. I was just asking if it made sense to add a rule to remove method block annotations as well. I assume not, but was asking more to get a better understanding of how these rules are used.

Presumably, there's no need for even this rule to exist going forward (running at again would be a NOP) -- so I might be misunderstanding the purpose of collecting these rules in the rector configuration.

Copy link
Member Author

Choose a reason for hiding this comment

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

The purpose of leaving these rules in place in the configuration file is to impose them on new developments or pending branches. If, for example, a developer forgets or ignores that @api annotations are no longer used, this will be corrected automatically before the code review.

@GromNaN GromNaN merged commit b4909e8 into mongodb:master Jun 1, 2023
@GromNaN GromNaN deleted the PHPLIB-1117 branch June 1, 2023 09:54
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.

2 participants