Skip to content

Comments

Add annotations#83

Closed
FrankGiesecke wants to merge 1 commit intowebmozarts:masterfrom
FrankGiesecke:add-annotations
Closed

Add annotations#83
FrankGiesecke wants to merge 1 commit intowebmozarts:masterfrom
FrankGiesecke:add-annotations

Conversation

@FrankGiesecke
Copy link

Add annotations to support benefits of modern IDE´s while consuming this package

Copy link
Contributor

@DavidGarciaCat DavidGarciaCat left a comment

Choose a reason for hiding this comment

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

Hey there,

I would like to know if maintainers believe that this update might be useful or not. Most of the methods (or all of them) are self-descriptive so I don't know if devs can get a real benefit of this PR.

Having said that, I believe that there are 2 things that could be reviewed for this PR:

  1. I would like to "request" the removal of the .idea folder - this is part of your local environment and it doesn't need to be the same than other developers are using.

  2. Also, I would like to see a bit more of consistency with the annotations.

@@ -1,2 +1,3 @@
/.idea/
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO the settings folder of your IDE such as .idea for PHPStorm or netbeans for NetBeans should always be included as part of your Global Git Ignore but never in a project's Git Ignore file.

Copy link
Author

Choose a reason for hiding this comment

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

I agree, although PHPStorm has established itself as a defacto standard.

Copy link
Contributor

@DavidGarciaCat DavidGarciaCat Jan 12, 2019

Choose a reason for hiding this comment

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

I had a few weeks ago, I had a discussion about IDEs with a colleague that basically said he has a love/hate relationship with PHPStorm. Also, until a few months ago, I was working with another developer who moved back to Netbeans due to PHPStorm was ”too slow”.

Although I love PHPStorm, I understand that not everyone wants to use it.

/**
* String.
*
* @param $value
Copy link
Contributor

Choose a reason for hiding this comment

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

For cases like this, I would suggest one of the following cases:

  • Add the annotation as @param mixed $value
  • Be consistent with the check that we're expecting to validate - for this case @param string $value

@BackEndTea BackEndTea mentioned this pull request Aug 7, 2019
@BackEndTea
Copy link
Collaborator

Thank you for your contribution, however, we have decied to merge #118 instead, which provides the annotations that are mentioned here.

@BackEndTea BackEndTea closed this Aug 24, 2019
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.

3 participants