Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

PR for Add clear docblock to all classes and interfaces #298

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

PR for Add clear docblock to all classes and interfaces #298

wants to merge 3 commits into from

Conversation

fadoe
Copy link

@fadoe fadoe commented Dec 28, 2017

This PR is for #297 and adds some docblocks for classes and interfaces.

@@ -33,7 +33,7 @@
protected $count = null;

/**
* @var Iterator|IteratorAggregate|ResultInterface
* @var Iterator|IteratorAggregate|ResultInterface|ArrayIterator
Copy link
Member

Choose a reason for hiding this comment

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

ArrayIterator is not really needed - the interfaces cover that

Copy link
Author

@fadoe fadoe Aug 7, 2018

Choose a reason for hiding this comment

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

You are right, but it is not in my file at https://github.com/fadoe/zend-db/blob/feature/docblock/src/ResultSet/AbstractResultSet.php#L36
Do you know why it is here?

/**
* Select
*
* @param Where|\Closure|string|array $where
Copy link
Member

Choose a reason for hiding this comment

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

Can it still be null?

/**
* Insert
*
* @param array $set
Copy link
Member

Choose a reason for hiding this comment

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

Add a description here

* Insert
*
* @param array $set
* @return int
Copy link
Member

Choose a reason for hiding this comment

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

Add a description here (number of affected rows)

/**
* Update
*
* @param array $set
Copy link
Member

Choose a reason for hiding this comment

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

Add a description here

* Update
*
* @param array $set
* @param string|array|\Closure $where
Copy link
Member

Choose a reason for hiding this comment

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

Can it still be null?

* @param array $set
* @param string|array|\Closure $where
*
* @return int
Copy link
Member

Choose a reason for hiding this comment

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

description

public function update($set, $where = null);

/**
* Delete
Copy link
Member

Choose a reason for hiding this comment

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

Overall, these one-word descriptions are not useful. Drop them overall unless there is something to clarify further

@ezimuel
Copy link
Contributor

ezimuel commented Aug 7, 2018

@fadoe can you finalize the changes requested by @Ocramius? Thanks!

@weierophinney
Copy link
Member

This repository has been moved to laminas/laminas-db. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

  • Squash all commits in your branch (git rebase -i origin/{branch})
  • Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
  • Run the laminas/laminas-migration tool on the code.
  • Clone laminas/laminas-db to another directory.
  • Copy the files from the second bullet point to the clone of laminas/laminas-db.
  • In your clone of laminas/laminas-db, commit the files, push to your fork, and open the new PR.
    We will be providing tooling via laminas/laminas-migration soon to help automate the process.

@michalbundyra
Copy link
Member

This repository has been closed and moved to laminas/laminas-db; a new issue has been opened at laminas/laminas-db#57.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants