Add :http_verbs option #50

hoppergee · 2021-07-19T14:49:05Z

Describe the change

Add :http_verbs option to breadcrumb parameters

Why are we doing this?

When I use this gem on some legacy projects, I met a circumstance which I have to show breadcrumbs on POST/PATCH action. I tried to add an option :http_only at #49. But @piotrmurach suggest me to use http_verbs.

Benefits

Now the current breadcrumb can match with multiple HTTP methods

breadcrumb 'Step 1', onboard_step_path(step: 1), http_verbs: [:get]
breadcrumb 'Step 2', onboard_step_path(step: 2), http_verbs: [:get, :post, :delete]
breadcrumb 'Step 3', onboard_step_path(step: 3), http_verbs: :all

Example: http_verbs: %w[get head] http_verbs: %w[post get head put patch delete] http_verbs: :all

Gemfile

lib/loaf/configuration.rb

lib/loaf/view_extensions.rb

spec/integration/breadcrumb_trail_spec.rb

piotrmurach

This is looking very promising! Thanks for a quick turnaround on this issue. 🙏 I left few comments. Could you please also use double-quoted strings everywhere? Sorry for the confusion, there is a mixture of both styles but going forward I'd like to keep them double-quoted.

README.md

lib/loaf/configuration.rb

lib/loaf/view_extensions.rb

Gemfile

README.md

lib/loaf/configuration.rb

lib/loaf/view_extensions.rb

spec/integration/breadcrumb_trail_spec.rb

spec/rails_app/app/controllers/onboard_controller.rb

spec/rails_app/config/routes.rb

piotrmurach

Things are looking good! Would you please address the remaining 'houndci' style issues? There are still some single-quoted strings left, arrays that can be converted to symbol arrays etc. Also, there are some tests failures in Rails 3.2. I know it's old Rails - sorry! But I don't want to break things in this release. I will consider dropping support later on in a separate release. Thank you!

lib/loaf/configuration.rb

spec/integration/breadcrumb_trail_spec.rb

spec/rails_app/config/routes.rb

spec/support/capybara.rb

spec/support/dummy_view.rb

spec/unit/view_extensions/breadcrumb_trail_spec.rb

piotrmurach

Thank you Hopper for making these changes! Things are looking really good. There is a small style issue left.

There is one dilemma that I'm 'debating' which is the name of the parameter :http_verbs. The verbs term is kind of inaccurate because these are not all verbs and also it is rather 'colloquial' usage. The HTTP rfc talks about HTTP methods. It feels more appropriate to use methods. I'm equally debating whether it should be request_methods or http_methods? What're your thoughts on this? I want to make sure we pick the best configuration name.

lib/loaf/view_extensions.rb

spec/integration/breadcrumb_trail_spec.rb

hoppergee · 2021-07-29T10:34:52Z

Thank you Hopper for making these changes! Things are looking really good. There is a small style issue left.

There is one dilemma that I'm 'debating' which is the name of the parameter :http_verbs. The verbs term is kind of inaccurate because these are not all verbs and also it is rather 'colloquial' usage. The HTTP rfc talks about HTTP methods. It feels more appropriate to use methods. I'm equally debating whether it should be request_methods or http_methods? What're your thoughts on this? I want to make sure we pick the best configuration name.

Maybe the request_methods is better, as Rack is using request_method.

https://github.com/rack/rack/blob/d15dd728440710cfc35ed155d66a98dc2c07ae42/lib/rack/request.rb#L174-L202

houndci-bot · 2021-10-12T04:21:45Z

lib/loaf/crumb.rb

    def initialize(name, url, options = {})
      @name  = name || raise_name_error
      @url   = url || raise_url_error
      @match = options.fetch(:match, Loaf.configuration.match)
+      @request_methods = options.fetch(:request_methods, Loaf.configuration.request_methods)


Metrics/LineLength: Line is too long. [92/80]

houndci-bot · 2021-10-12T04:21:45Z

lib/loaf/view_extensions.rb

+        current = current_crumb?(
+          path,
+          options.fetch(:match) { crumb.match },
+          request_methods: options.fetch(:request_methods) { crumb.request_methods }


Metrics/LineLength: Line is too long. [84/80]

piotrmurach

Apologies for not having replied to your request for review earlier. I took a break from open-source. I left a few comments. Let's move this PR forward.

README.md

lib/loaf/view_extensions.rb

spec/integration/breadcrumb_trail_spec.rb

spec/unit/view_extensions/breadcrumb_trail_spec.rb

hoppergee · 2022-02-22T04:28:27Z

Apologies for not having replied to your request for review earlier. I took a break from open-source. I left a few comments. Let's move this PR forward.

Glad to hear that! I'll get it done sometime this week.

houndci-bot · 2022-02-25T10:26:53Z

lib/loaf/view_extensions.rb

@@ -65,8 +69,8 @@ def breadcrumb_trail(options = {})
    #   the pattern to match on
    #
    # @api public
-    def current_crumb?(path, pattern = :inclusive)
-      return false unless request.get? || request.head?
+    def current_crumb?(path, pattern = :inclusive, request_methods: Loaf.configuration.request_methods)


Metrics/LineLength: Line is too long. [103/80]

spec/integration/breadcrumb_trail_spec.rb

spec/integration/configuration_spec.rb

hoppergee · 2022-02-25T10:36:22Z

I just fixed all those style issues. Also fixed a wrong proc config test issue

houndci-bot · 2022-02-25T10:42:56Z

spec/rails_app/app/controllers/comments_controller.rb

@@ -3,10 +3,12 @@ class Article < Struct.new(:id, :title); end
 class CommentsController < ApplicationController

  breadcrumb lambda { |c| c.find_article(c.params[:post_id]).title },
-             lambda { |c| c.post_comments_path(c.params[:post_id]) }
+             lambda { |c| c.post_comments_path(c.params[:post_id]) },


Style/Lambda: Use the -> { ... } lambda literal syntax for single line lambdas.

ankalus · 2023-05-04T09:32:31Z

Are there plans to merge these changes?

hoppergee added 4 commits July 19, 2021 22:21

Add http_verbs option

a491c87

Example: http_verbs: %w[get head] http_verbs: %w[post get head put patch delete] http_verbs: :all

Add integration tests for http_verbs

288cb8c

Add byebug and rspec focus

bbea64a

Update README for :http_verbs

47d50b7

houndci-bot reviewed Jul 19, 2021

View reviewed changes

hoppergee mentioned this pull request Jul 19, 2021

Add get_only option #49

Closed

piotrmurach reviewed Jul 19, 2021

View reviewed changes

hoppergee added 5 commits July 20, 2021 19:53

Remove byebug

989462d

Change http_verbs default value to [:get, :head]

2299ded

Fix some ruby style issues

f94f272

Refactor ViewExtensions#current_crumb? to use try

9fa3f45

Add more http_verbs options in README

b8478a0

houndci-bot reviewed Jul 20, 2021

View reviewed changes

hoppergee requested a review from piotrmurach July 20, 2021 13:09

piotrmurach reviewed Jul 20, 2021

View reviewed changes

lib/loaf/configuration.rb Outdated Show resolved Hide resolved

Fix more ruby style issues

aa45ffa

houndci-bot reviewed Jul 28, 2021

View reviewed changes

Turn off PATCH test when running test for rails3.2

64aa93d

hoppergee force-pushed the http_verbs_option branch from 4680972 to 64aa93d Compare July 28, 2021 12:03

Fix some style issues

e28ea0f

hoppergee force-pushed the http_verbs_option branch from 9a033f2 to e28ea0f Compare July 28, 2021 12:19

hoppergee requested a review from piotrmurach July 28, 2021 12:22

piotrmurach reviewed Jul 28, 2021

View reviewed changes

lib/loaf/view_extensions.rb Outdated Show resolved Hide resolved

spec/integration/breadcrumb_trail_spec.rb Show resolved Hide resolved

spec/integration/breadcrumb_trail_spec.rb Outdated Show resolved Hide resolved

Change http_verbs to request_methods

72b1590

houndci-bot reviewed Oct 12, 2021

View reviewed changes

hoppergee requested a review from piotrmurach November 2, 2021 10:37

piotrmurach reviewed Feb 21, 2022

View reviewed changes

piotrmurach linked an issue Feb 21, 2022 that may be closed by this pull request

Crumbs are never current on POST #51

Open

piotrmurach mentioned this pull request Feb 21, 2022

Crumbs are never current on POST #51

Open

houndci-bot reviewed Feb 25, 2022

View reviewed changes

hoppergee force-pushed the http_verbs_option branch 2 times, most recently from ba3602e to c5d8971 Compare February 25, 2022 10:33

Fix more style issues

b94519a

hoppergee force-pushed the http_verbs_option branch from c5d8971 to b94519a Compare February 25, 2022 10:35

Fix a wrong proc config test

c074e6d

houndci-bot reviewed Feb 25, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add :http_verbs option #50

Add :http_verbs option #50

hoppergee commented Jul 19, 2021

piotrmurach left a comment

piotrmurach left a comment

piotrmurach left a comment

hoppergee commented Jul 29, 2021

houndci-bot Oct 12, 2021

houndci-bot Oct 12, 2021

piotrmurach left a comment

hoppergee commented Feb 22, 2022

houndci-bot Feb 25, 2022

hoppergee commented Feb 25, 2022 •

edited

houndci-bot Feb 25, 2022

ankalus commented May 4, 2023

Add :http_verbs option #50

Are you sure you want to change the base?

Add :http_verbs option #50

Conversation

hoppergee commented Jul 19, 2021

Describe the change

Why are we doing this?

Benefits

piotrmurach left a comment

Choose a reason for hiding this comment

piotrmurach left a comment

Choose a reason for hiding this comment

piotrmurach left a comment

Choose a reason for hiding this comment

hoppergee commented Jul 29, 2021

houndci-bot Oct 12, 2021

Choose a reason for hiding this comment

houndci-bot Oct 12, 2021

Choose a reason for hiding this comment

piotrmurach left a comment

Choose a reason for hiding this comment

hoppergee commented Feb 22, 2022

houndci-bot Feb 25, 2022

Choose a reason for hiding this comment

hoppergee commented Feb 25, 2022 • edited

houndci-bot Feb 25, 2022

Choose a reason for hiding this comment

ankalus commented May 4, 2023

hoppergee commented Feb 25, 2022 •

edited