Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Validator] NewDivisibleBy constraint for testing divisibility#28069

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@colinodell
Copy link
Contributor

@colinodellcolinodell commentedJul 26, 2018•
edited
Loading

This introduces a newMultipleOfDivisibleBy constraint which checks whether one number is a multiple of (aka "divisible by") some other number. Useful for enforcing specific increments on a number.

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsn/a
LicenseMIT
Doc PRsymfony/symfony-docs#10121

Seesymfony/symfony-docs#10121 for examples of this constraint in action.

semors and COil reacted with thumbs up emoji
@colinodellcolinodellforce-pushed thefeature/multiple-of-validator branch fromb96bb3a tof8cf896CompareJuly 26, 2018 19:39
@colinodellcolinodell changed the titleImplement validation constraint for testing whether divisibility[Validator] NewMultipleOf constraint for testing divisibilityJul 26, 2018
@colinodellcolinodellforce-pushed thefeature/multiple-of-validator branch fromf8cf896 to36e369eCompareJuly 26, 2018 19:41
@colinodell
Copy link
ContributorAuthor

The AppVeyor failure seems to be unrelated to this change.

@javiereguiluzjaviereguiluz added this to thenext milestoneJul 27, 2018
Copy link
Member

@weaverryanweaverryan left a comment

Choose a reason for hiding this comment

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

Nice job Colin! I just tested this in a real project - no missing pieces - it works great :).

@colinodell
Copy link
ContributorAuthor

Thanks Ryan! I'm currently using this in an application with a "time spent" field which only accepts 0.25 hour increments. I feel like having amodulus == 0 constraint like this would round out Symfony's other comparison constrains nicely.

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedAug 10, 2018•
edited
Loading

What about renaming toDivisibleBy? I first readMultipleOf as something likeSeveralOf, which made no sense.

dmaicher, xabbuh, colinodell, apfelbox, yceruto, and jennik reacted with thumbs up emoji

@xabbuh
Copy link
Member

I agree with Nicolas. And since the test in Twig is named the same that would feel more consistent.

@colinodell
Copy link
ContributorAuthor

Thanks for the feedback@nicolas-grekas! I have renamed the constraint and updated the docs PR accordingly.

@colinodellcolinodell changed the title[Validator] NewMultipleOf constraint for testing divisibility[Validator] NewDivisibleBy constraint for testing divisibilityAug 10, 2018
self::NOT_DIVISIBLE_BY => 'NOT_MULTIPLE_OF',
);

public $message = 'This value should be divisible by {{ compared_value }}.';
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to also add translations for this string.

Copy link
Contributor

@curry684curry684Aug 10, 2018•
edited
Loading

Choose a reason for hiding this comment

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

The message is also not all that clear from a user's perspective. To a developer "8 is divisible by 2" makes sense, but a mathematician would say "so is 7.6543", and a common user would just be like "erm keep the math out of my way".

I'd stick with the more genericThis value must be a multiple of {{ compared_value }}. for the end-user feedback.

xabbuh, javiereguiluz, colinodell, and apfelbox reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I have updated the message as requested and force-pushed the changes.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I have also added translation strings in English and Spanish.

const NOT_DIVISIBLE_BY = '6d99d6c3-1464-4ccf-bdc7-14d083cf455c';

protected static $errorNames = array(
self::NOT_DIVISIBLE_BY => 'NOT_MULTIPLE_OF',
Copy link
Contributor

Choose a reason for hiding this comment

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

Forgot to update the constant value.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed and force-pushed. Thanks for catching this!

@colinodellcolinodellforce-pushed thefeature/multiple-of-validator branch 4 times, most recently frombb9feb7 to6eff196CompareAugust 10, 2018 17:03
@curry684
Copy link
Contributor

For Dutch (nl):

            <trans-unitid="84">                <source>This value should be a multiple of {{ compared_value }}.</source>                <target>Deze waarde moet een veelvoud zijn van {{ compared_value }}.</target>            </trans-unit>

@dmaicher
Copy link
Contributor

dmaicher commentedAug 13, 2018•
edited
Loading

For German (de):

Dieser Wert sollte ein Vielfaches von {{ compared_value }} sein.

@xabbuh
Copy link
Member

@dmaicher Though we need to capitalise theV (seehttps://www.duden.de/rechtschreibung/Vielfaches):

Dieser Wert sollte ein Vielfaches von {{ compared_value }} sein.

dmaicher reacted with thumbs up emojidmaicher and DavidBadura reacted with laugh emoji

@colinodell
Copy link
ContributorAuthor

I have added the two translations (but now fabbot is failing due to an existing translation)

@curry684
Copy link
Contributor

The correction is wrong.@fabpot you should probably exempt.xlf files from typo checks in fabbot if it only searches for English typos 😉

*/
protected function compareValues($value1, $value2)
{
return 0 == fmod($value1, $value2);

Choose a reason for hiding this comment

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

let's use strict comparison here

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

For french:
Cette valeur doit être un multiple de {{ compared_value }}.

@colinodell
Copy link
ContributorAuthor

Thanks for the feedback@nicolas-grekas! I have made the comparison strict and added the French translation.

@nicolas-grekasnicolas-grekasforce-pushed thefeature/multiple-of-validator branch from3993781 toefcfb8bCompareAugust 15, 2018 15:44
@nicolas-grekasnicolas-grekas merged commitefcfb8b intosymfony:masterAug 15, 2018
nicolas-grekas added a commit that referenced this pull requestAug 15, 2018
…ivisibility (colinodell)This PR was squashed before being merged into the 4.2-dev branch (closes#28069).Discussion----------[Validator] New `DivisibleBy` constraint for testing divisibilityThis introduces a new ~`MultipleOf`~ `DivisibleBy` constraint which checks whether one number is a multiple of (aka "divisible by") some other number.  Useful for enforcing specific increments on a number.| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | n/a| License       | MIT| Doc PR        |symfony/symfony-docs#10121Seesymfony/symfony-docs#10121 for examples of this constraint in action.Commits-------efcfb8b [Validator] New `DivisibleBy` constraint for testing divisibility
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull requestSep 6, 2018
…ell)This PR was merged into the master branch.Discussion----------[Validator] Document the DivisibleBy constraintDocumentation for the new ~`MultipleOf`~ `DivisibleBy` constraint proposed insymfony/symfony#28069Commits-------3270cca Document the DivisibleBy constraint
@nicolas-grekasnicolas-grekas modified the milestones:next,4.2Nov 1, 2018
This was referencedNov 3, 2018
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@weaverryanweaverryanweaverryan approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@xabbuhxabbuhxabbuh approved these changes

+1 more reviewer

@curry684curry684curry684 requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

8 participants

@colinodell@nicolas-grekas@xabbuh@curry684@dmaicher@weaverryan@javiereguiluz@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp