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

[RateLimiter] Rename RateLimiter to RateLimiterFactory#38675

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

Merged
chalasr merged 1 commit intosymfony:5.xfromNyholm:ratelimiter-rename-factory
Oct 24, 2020

Conversation

@Nyholm
Copy link
Member

@NyholmNyholm commentedOct 22, 2020•
edited
Loading

QA
Branch?5.x
Bug fix?no
New feature?no
Deprecations?No, not released yet
Tickets
LicenseMIT
Doc PRshould be added

Sorry for making a few BC breaks.

@wouterjsaid that this class was suggested to be namedLimiterFactory before. But that was rejected.

Just my looking at the names of the classes we currently have:

  • Rate
  • RateLimit
  • RateLimiter

I find it hard to know what these are doing and the difference between them. Note that none of them are used as a rate limiter (ie implementsLimiterInterface)

I would like to be clear that aRateLimiterFactory is used to create an object implementingLimiterInterface.

@stof
Copy link
Member

Such renaming makes sense to me

@jderussejderusse changed the title[RateLimiter] Rename RateLimiter to RateLimiter factory[RateLimiter] Rename RateLimiter to RateLimiterFactoryOct 22, 2020
@NyholmNyholmforce-pushed theratelimiter-rename-factory branch from52b3e0d toc08202aCompareOctober 22, 2020 10:17
Copy link
Member

@javiereguiluzjaviereguiluz left a comment

Choose a reason for hiding this comment

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

The reviewers suggestions should probably be taken into account, but this looks good! Thanks!

@Nyholm
Copy link
MemberAuthor

Thank you. PR is updated.

@chalasr
Copy link
Member

@Nyholm Why did you revert the service renaming?

@Nyholm
Copy link
MemberAuthor

Because the tests was failing all over the place. :(

Thelimiter service is abstract and all ourLimiterInterface services uses that abstract service. They are created with nameslimiter.acme. Whereacme is just the thing added to exiting service. So they would be namedlimiter_factory.acme.

I tried searching for all references in other components and fixing the issue but I couldn't really. I can try an another search if you want.

@chalasr
Copy link
Member

Works for me as-is. Thanks for explaining :)

@chalasrchalasrforce-pushed theratelimiter-rename-factory branch from8d120d3 to8be261bCompareOctober 24, 2020 08:11
@chalasr
Copy link
Member

Thank you Tobias.

@chalasrchalasr merged commit1c81aa7 intosymfony:5.xOct 24, 2020
@Nyholm
Copy link
MemberAuthor

Thank you for merging

@NyholmNyholm deleted the ratelimiter-rename-factory branchOctober 24, 2020 08:13
@fabpotfabpot mentioned this pull requestOct 28, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@jderussejderussejderusse approved these changes

@chalasrchalasrchalasr approved these changes

@javiereguiluzjaviereguiluzjaviereguiluz approved these changes

@wouterjwouterjAwaiting requested review from wouterj

Assignees

No one assigned

Projects

None yet

Milestone

5.2

Development

Successfully merging this pull request may close these issues.

6 participants

@Nyholm@stof@chalasr@javiereguiluz@jderusse@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp