Skip to content

Conversation

thiemowmde
Copy link
Contributor

@thiemowmde thiemowmde commented Dec 28, 2015

I found that all these changes are to minor for individual patches. They also interfere with each other. If you disagree with a change but want to merge the rest, please tell me, and I will split it off.

  • Make protected stuff private by default. We already did this almost everywhere.
  • Remove since tags from private stuff.
  • Make @param tags as specific as possible.
  • Avoid full qualified class names.
  • Remove a not needed @var tag.
  • Fix my name.
  • Update license tags according to SPDX version 3.
  • Start using the PHPUnit's forward-compatible TestCase alias.
  • Drop the custom PHPCS rule set, and use the upstream Wikibase rule set instead.

*/
protected $allowAuto = false;
private $allowAuto = false;
Copy link
Member

Choose a reason for hiding this comment

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

this is a breaking change, cannot be done before next breaking rel, and might not make sense in there

Wikibase is not the only user of this lib btw

@JeroenDeDauw
Copy link
Member

-2 as per inline comment

@thiemowmde
Copy link
Contributor Author

Do you prefer this to be split in a separate patch, or not done at all?

@JeroenDeDauw
Copy link
Member

The cost benefit of doing this seems very dubious. You need a breaking rel which is certainly not justified by just that change and need to verify such a change is fine for all users.

@thiemowmde
Copy link
Contributor Author

I reverted all the protected/private and @since stuff for now.

@JeroenDeDauw
Copy link
Member

+0

@mariushoch
Copy link
Member

Needs rebase.

@thiemowmde
Copy link
Contributor Author

Rebased and updated (see above).

@mariushoch mariushoch merged commit 1337895 into master Mar 15, 2018
@mariushoch mariushoch deleted the fullQualified branch March 15, 2018 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants