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

breadcrumbs_trail misbehaves if item identifiers have more than one "." #1278

Closed
iay opened this Issue Dec 20, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@iay
Contributor

iay commented Dec 20, 2017

Items with more than one "." in their identifier (e.g., "1.6.md") can cause breadcrumbs_trail to reference the wrong item.

Steps to reproduce

Make items like this:

/trp/1.5.md
/trp/1.5/foo.md
/trp/1.6.md
/trp/1.6/foo.md

The breadcrumbs for one or other of the foo.md items will refer to the wrong parent.

Expected behavior

I'd expect the last element of the breadcrumb trail for an item to always be that item's parent.

Actual behavior

Sometimes the last element of the breadcrumb trail for an item can be an item with a similar name.

Details

It looks like the breadcrumbs_trail method for full identifiers is stripping extensions for identifier components other than the last: here

@items[Nanoc::Identifier.new(pr).without_ext + '.*']

For example, for /trp/1.6/software.md:

  • components becomes ["", "/trp", "/trp/1.6", "/trp/1.6/software.md"]

  • The identifiers used to construct the prefixes array are: ["/index.*", "/trp.*", "/trp/1.*", "/trp/1.6/software.*"]

Note that "/trp/1.*" can match either /trp/1.5.md or /trp/1.6.md.

It seems to me as if stripping the extension isn't required here, as long as you don't add the .* on to the end of the last identifier in the list: that one will already be present as an explicit extension if we're working with full identifiers.

I could be missing something, though, I am really not a native Ruby programmer (yet).

@iay iay changed the title from breadcrumbs_trail misbehaves if item identifiers have more than "." to breadcrumbs_trail misbehaves if item identifiers have more than one "." Dec 20, 2017

@iay

This comment has been minimized.

Show comment
Hide comment
@iay

iay Dec 20, 2017

Contributor

Changing this:

        prefixes
          .reject { |pr| pr =~ /^\/index\./ }
          .map do |pr|
            if pr == ''
              @items['/index.*']
            else
              @items[Nanoc::Identifier.new(pr).without_ext + '.*']
            end
          end

to this:

        prefixes.push(Nanoc::Identifier.new(prefixes.pop).without_ext)
        prefixes
          .reject { |pr| pr =~ /^\/index\./ }
          .map do |pr|
            if pr == ''
              @items['/index.*']
            else
              @items[Nanoc::Identifier.new(pr) + '.*']
            end
          end
      end

... appears to fix the problem in my case, by only stripping the extension from the last prefix (representing the current item itself). I doubt that it's idiomatic Ruby, and I can't find any unit tests to run to see if it would break something else. But it's a start. I can set up a PR if you want one.

Contributor

iay commented Dec 20, 2017

Changing this:

        prefixes
          .reject { |pr| pr =~ /^\/index\./ }
          .map do |pr|
            if pr == ''
              @items['/index.*']
            else
              @items[Nanoc::Identifier.new(pr).without_ext + '.*']
            end
          end

to this:

        prefixes.push(Nanoc::Identifier.new(prefixes.pop).without_ext)
        prefixes
          .reject { |pr| pr =~ /^\/index\./ }
          .map do |pr|
            if pr == ''
              @items['/index.*']
            else
              @items[Nanoc::Identifier.new(pr) + '.*']
            end
          end
      end

... appears to fix the problem in my case, by only stripping the extension from the last prefix (representing the current item itself). I doubt that it's idiomatic Ruby, and I can't find any unit tests to run to see if it would break something else. But it's a start. I can set up a PR if you want one.

@iay

This comment has been minimized.

Show comment
Hide comment
@iay

iay Dec 20, 2017

Contributor

Alas, that quick fix does indeed run into an additional problem with the /index.md item, as it is no longer matched in the .reject clause, so you end up with two references to itself from the root item.

This variant appears to address that:

        prefixes.push(Nanoc::Identifier.new(prefixes.pop).without_ext)
        prefixes
          .reject { |pr| pr =~ /^\/index$/ }
          .map do |pr|
            if pr == ''
              @items['/index.*']
            else
              @items[Nanoc::Identifier.new(pr) + '.*']
            end
          end
Contributor

iay commented Dec 20, 2017

Alas, that quick fix does indeed run into an additional problem with the /index.md item, as it is no longer matched in the .reject clause, so you end up with two references to itself from the root item.

This variant appears to address that:

        prefixes.push(Nanoc::Identifier.new(prefixes.pop).without_ext)
        prefixes
          .reject { |pr| pr =~ /^\/index$/ }
          .map do |pr|
            if pr == ''
              @items['/index.*']
            else
              @items[Nanoc::Identifier.new(pr) + '.*']
            end
          end
@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Dec 21, 2017

Member

I believe that the purpose of #without_ext was to support items with multiple extensions, like these:

/foo/stuff.md
/foo.md.erb
Member

ddfreyne commented Dec 21, 2017

I believe that the purpose of #without_ext was to support items with multiple extensions, like these:

/foo/stuff.md
/foo.md.erb
@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Dec 21, 2017

Member

There is a fix in #1279 with some fancy functional programming

Member

ddfreyne commented Dec 21, 2017

There is a fix in #1279 with some fancy functional programming

@ddfreyne ddfreyne closed this in #1279 Dec 22, 2017

ddfreyne added a commit that referenced this issue Dec 22, 2017

Merge pull request #1279 from nanoc/gh-1278
Let #breadcrumbs_trail always find closest parent
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment