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

25076 finder should optionally check other time types #25084

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@simonhayre
Copy link

simonhayre commented Nov 21, 2017

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #25076
License MIT
Doc PR symfony/symfony-docs#8715
@Simperfit

This comment has been minimized.

Copy link
Contributor

Simperfit commented Dec 11, 2017

Could you please rebase and look why are the test passing ?

@simonhayre simonhayre force-pushed the simonhayre:25076-finder-should-optionally-check-other-time-types branch 4 times, most recently from f3487de to 19aec77 Jan 2, 2018

@@ -136,13 +136,89 @@ public function depth($level)
*
* @return $this
*
* @deprecated since 4.1, to be removed in 4.2

This comment has been minimized.

@fabpot

fabpot Jan 3, 2018

Member

should to be removed in 5.0.

*
* The date must be something that strtotime() is able to parse:
*
* $finder->date('since yesterday');

This comment has been minimized.

@javiereguiluz

javiereguiluz Jan 3, 2018

Member

The examples in this PHPdoc should be updated to replace ->date(... with the new methods.

Show resolved Hide resolved src/Symfony/Component/Finder/Finder.php

@simonhayre simonhayre force-pushed the simonhayre:25076-finder-should-optionally-check-other-time-types branch from 19aec77 to 80a922e Jan 3, 2018

@simonhayre simonhayre force-pushed the simonhayre:25076-finder-should-optionally-check-other-time-types branch from 80a922e to 68c122e Jan 3, 2018

@@ -136,13 +136,104 @@ public function depth($level)
*
* @return $this
*
* @deprecated since 4.1, to be removed in 5.0

This comment has been minimized.

@simonhayre

simonhayre Jan 3, 2018

now changed to

should to be removed in 5.0.

This comment has been minimized.

@fabpot

fabpot Jan 17, 2018

Member

Re-reading this PR, I think date() should be keep as is. Using the modified date is the most useful comparison, so it deserves to be the default.

*
* @throws \InvalidArgumentException If the test is not understood
*/
public function __construct(string $test)
public function __construct($test, $timeType = self::TIME_TYPE_MODIFIED)

This comment has been minimized.

@fabpot

fabpot Jan 17, 2018

Member

Why have you removed the string type hint?

This comment has been minimized.

@fabpot

fabpot Jan 17, 2018

Member

$timeType can be type-hinted with string

const TIME_TYPE_CHANGED = 'C';
const TIME_TYPE_ACCESSED = 'A';
/** @var string */

This comment has been minimized.

@fabpot

fabpot Jan 17, 2018

Member

Can be removed

@@ -136,13 +136,104 @@ public function depth($level)
*
* @return $this
*
* @deprecated since 4.1, to be removed in 5.0

This comment has been minimized.

@fabpot

fabpot Jan 17, 2018

Member

Re-reading this PR, I think date() should be keep as is. Using the modified date is the most useful comparison, so it deserves to be the default.

* @see DateRangeFilterIterator
* @see DateComparator
*/
public function dateModified($date)

This comment has been minimized.

@fabpot

fabpot Jan 17, 2018

Member

Can be removed as date() is kept.

@nicolas-grekas nicolas-grekas modified the milestones: 4.1, next Apr 20, 2018

/**
* @return string
*/

This comment has been minimized.

@fabpot

fabpot Sep 4, 2018

Member

Docblocks that do not add value should be removed. This one should be removed as the method itself already gives all the info (same for __construct).

* @see DateRangeFilterIterator
* @see DateComparator
*/
public function dateAccessed($date)

This comment has been minimized.

@fabpot

fabpot Sep 4, 2018

Member

Instead of create new methods, what about adding an argument to the date one?

This comment has been minimized.

@simonhayre

simonhayre Jan 10, 2019

I've added an argument to date method and called the same method from these.
I feel that it's clearer to the user to have separate methods with documentation to describe what the difference is between each method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment