Skip to content
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

create a constant with pagination value #1130

Closed
wants to merge 4 commits into from
Closed

create a constant with pagination value #1130

wants to merge 4 commits into from

Conversation

lucianosousa
Copy link
Contributor

avoid duplicated values in the controller

@nateberkopec
Copy link
Contributor

👍 Is this value used anywhere else?

@lucianosousa
Copy link
Contributor Author

lucianosousa commented Nov 30, 2015

Couldn't find it, but didn't look too deep.
I saw in a lot of places a pagination by 5 items.

@simi
Copy link
Member

simi commented Nov 30, 2015

@lucianosousa
Copy link
Contributor Author

Should we create a constant for this value in application.rb or any other place?

@farukaydin
Copy link
Member

👎 on this. Although it's a kind of reducing duplication, it's not worth to change since we have different number of pagination and current version is sufficient enough to keep the codebase more readable.

Though, thanks for contributing rubygems.org 💙 ❤️

@sonalkr132
Copy link
Member

sonalkr132 commented Jan 3, 2017

I think this makes sense, we should have as few magic numbers as possible, even if it is only 2/3 fewer.

Should we create a constant for this value in application.rb

application.rb should be fine.

@lucianosousa
Copy link
Contributor Author

Good.
I'll work on this still this week. ;)

@jvanbaarsen
Copy link
Contributor

@lucianosousa Are you still working on this? If not, can you please close the PR? That way the list of open PRs is kept organised with active ones only :-)

@lucianosousa
Copy link
Contributor Author

@jvanbaarsen sorry being late here Sir. It's done!

@jvanbaarsen
Copy link
Contributor

@dwradcliffe I think this is ready to merge :-)

@sonalkr132
Copy link
Member

sonalkr132 commented Apr 15, 2017

@lucianosousa can you please squash your commits?

@sonalkr132
Copy link
Member

Merged as f5686b3.

@sonalkr132 sonalkr132 closed this Apr 17, 2017
@lucianosousa lucianosousa deleted the patch-2 branch April 17, 2017 13:10
@lucianosousa
Copy link
Contributor Author

cheers mates

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

Successfully merging this pull request may close these issues.

None yet

7 participants