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

FIX: missing dup on blocks #203

Merged
merged 1 commit into from
Oct 14, 2019
Merged

Conversation

rogercampos
Copy link
Contributor

The dup of the blocks argument was removed on the last refactor of the route_set, but it was necessary. On the other hand, defaults doesn't need a dup since this is already done inside ::ActionDispatch::Routing::Mapper::Mapping.build.

The new test showcases the bug (404 response) with the previous code.

@coveralls
Copy link

coveralls commented Oct 14, 2019

Coverage Status

Coverage increased (+0.0004%) to 99.314% when pulling 1d8db83 on rogercampos:fix/dup into a5d9097 on enriclluelles:master.

@tagliala
Copy link
Collaborator

tagliala commented Oct 14, 2019

@rogercampos thanks for this PR

I need just two things:

  1. Check that the dup is done in each supported version of Rails
  2. I need to upgrade contributing guidelines, but please change your commit message to
Fix missing dup on blocks

The dup of the `blocks` argument was removed on the last refactor of the
route_set, but it was necessary. On the other hand, `defaults` doesn't need a
dup since this is already done inside
`::ActionDispatch::Routing::Mapper::Mapping.build`.

The new test showcases the bug (404 response) with the previous code.

Ref: https://chris.beams.io/posts/git-commit/


de-AT:
routes:
dummy: Attrappe
show: anzeigen
account: account_de

Choose a reason for hiding this comment

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

konto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx!

The dup of the `blocks` argument was removed on the last refactor of the
route_set, but it was necessary. On the other hand, `defaults` doesn't need a
dup since this is already done inside
`::ActionDispatch::Routing::Mapper::Mapping.build`.

The new test showcases the bug (404 response) with the previous code.
@rogercampos
Copy link
Contributor Author

@tagliala done, I checked rails versions 5-0-stable, 5-1, 5-2 and 6-0, they all dup the defaults

@tagliala tagliala merged commit 5a5aa00 into enriclluelles:master Oct 14, 2019
@tagliala tagliala added the bug label Oct 14, 2019
@tagliala tagliala added this to the 7.0.1 milestone Oct 14, 2019
@rogercampos rogercampos deleted the fix/dup branch October 15, 2019 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants