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

Let breadcrumbs trail deal with ambiguity #1368

Merged
merged 4 commits into from Dec 1, 2018

Conversation

Projects
None yet
2 participants
@ddfreyne
Member

ddfreyne commented Oct 8, 2018

This makes #breadcrumbs_trail able to deal with ambiguity.

Detailed description

Given e.g. the items

  • /foo/stuff.md
  • /foo.md.erb
  • /foo.md
  • /foo.erb
  • /index.md

When requesting the breadcrumbs trail for /foo/stuff.md, the second item in the breadcrumbs trail might be /foo.md.erb, or /foo.md, or /foo.erb — and #breadcrumbs_trail cannot know which one is the one to be used.

At the moment, the breadcrumbs trail will contain a random one of them. This isn’t great, because it’s nondeterministic, and this case is probably erroneous.

By default, #breadcrumbs_trail will now print a warning:

Warning: The breadcrumbs trail (generated by #breadcrumbs_trail) found more than one
potential parent item at /software.* (found /software.erb and /software.md). Nanoc will
pick the first item as the parent. Consider eliminating the ambiguity by making only
one item match /software.*, or by passing a `:tiebreaker` option to `#breadcrumbs_trail`.
(This situation will be an error in the next major version of Nanoc.)

The new :tiebreaker option can be used to pick the desired item:

# random! because why not
breadcrumbs_trail(tiebreaker: -> (items) { items.sample })

# the second argument is the pattern, and is optional
breadcrumbs_trail(tiebreaker: -> (items, _pattern) { items.sample })

The :tiebreaker option can also be given the :error value, which will raise an error:

breadcrumbs_trail(tiebreaker: :error)
# Nanoc::Helpers::Breadcrumbs::AmbiguousAncestorError
# expected only one item to match /foo.*, but found 3'

To do

  • Tests
  • Documentation

Related issues

n/a

@iay

This comment has been minimized.

Contributor

iay commented Oct 9, 2018

In addition to detecting the case, it would be useful to provide a way of indicating how to break ties (for example by allowing an item attribute with the name of the correct parent item) so that a site with such an ambiguity can be made to work again.

@ddfreyne ddfreyne changed the base branch from master to trunk Oct 10, 2018

@ddfreyne

This comment has been minimized.

Member

ddfreyne commented Oct 10, 2018

Hmm, maybe a tiebreaker: option? e.g.

# pick random parent (probably not great)
breadcrumbs_trail(tiebreaker: -> (candidates) { candidates.sample })

# pick first parent (probably not great either)
breadcrumbs_trail(tiebreaker: -> (candidates) { candidates.first })

# use .md files as identifier
# (if none are found, raise an exception)
breadcrumbs_trail(tiebreaker: -> (candidates) { candidates.find { |c| c.identifier =~ '**/*.md' }})

What do you think?

@iay

This comment has been minimized.

Contributor

iay commented Oct 11, 2018

If you want to define breadcrumbs around the sequence of components you get from splitting an item's path, that looks like a good solution for most people. Looking at my own use site, though, I'm not sure if I would use that mechanism, and just rely on it to flag errors.

This may be a bit of a digression, but my own breadcrumbs issues (in my personal site, not other work) are more complex because of some history which means that the breadcrumb list for an item isn't always directly related to the series of components you get when splitting an item's path: sometimes there's an extra item that needs to be inserted out of hierarchy and some items just have completely different requirements because they live at paths that are entirely unrelated to

So the breadcrumbs system on my personal site uses a couple of attributes to:

  • Bypass breadcrumbs_trail entirely for items with a :crumbs attribute, and
  • Insert an extra element to what is returned from breadcrumbs_trail for items with a :crumb_parent attribute.

Sometimes I think that a breadcrumbs implementation that was based entirely on iteratively finding a "crumb parent" for the current item using a strategy function might be better: in my case, the strategy function would use :crumb_parent if present, then fall back to unwinding levels of hierarchy until it found a .md item. That's probably a lot slower than the path-splitting you do in your implementation, but it's probably only called once per page so I don't know if that matters.

Digression ends.

@iay

This comment has been minimized.

Contributor

iay commented Oct 11, 2018

FYI, this is what I put as a note to myself in the README.md of my site repository. You may be able to see this in the GitLab repository I gave you access to a while back, but they are having replication issues at the moment so I don't know how up-to-date it is.


  • crumbs: For items which are very far in the menu hierarchy from their position
    in the path hierarchy (which would normally turn into their breadcrumb
    path), this attribute is used to hold an array of item identifiers for
    the item's breadcrumbs. To reduce the amount of typing required, /index.*
    will be added to the front of this list, and the current item will be added
    to the end. Identifiers can include wildcards (e.g., /foo/bar.*).

  • crumb_parent: Indicates that the item has a breadcrumb parent other than the one implied
    by hierarchy. Used in content imported from Drupal; new item paths should
    always be created in an appropriate hierarchy.

@ddfreyne ddfreyne changed the base branch from trunk to master Oct 13, 2018

@ddfreyne ddfreyne force-pushed the breadcrumbs-less-ambiguity branch from dfc721c to c2af7ef Dec 1, 2018

@ddfreyne ddfreyne force-pushed the breadcrumbs-less-ambiguity branch from c2af7ef to 4277d9e Dec 1, 2018

@ddfreyne

This comment has been minimized.

Member

ddfreyne commented Dec 1, 2018

@iay I like the idea of being able to customise the way the breadcrumb chid-parent relationship is set up. I’ll play around with a parent_finder: -> (item, items) { … } function.

I’ve added a :tiebreaker option to #breadcrumbs_trail, which gives flexibility in how to handle ambiguous parent items.


I’m unsure what the default behavior of ambiguous parent items should be.

I’m a fan of error-by-default, but that might make this a non-backwards-compatible change. I say “might” rather than “would,” because the current behavior is undefined, and I believe that changing undefined behavior is backwards-compatible.

A better option is likely to log a warning along these lines:

Warning: The breadcrumbs trail (generated by #breadcrumbs_trail) found more than one
potential parent item at /software.* (found /software.erb and /software.md). Nanoc will
pick the first item as the parent. Consider eliminating the ambiguity by making only
one item match /software.*, or by passing a `:tiebreaker` option to `#breadcrumbs_trail`.
(This situation will be an error in the next major version of Nanoc.)

ddfreyne added some commits Dec 1, 2018

@ddfreyne ddfreyne changed the title from Let breadcrumbs trail refuse to deal with ambiguity to Let breadcrumbs trail deal with ambiguity Dec 1, 2018

@ddfreyne

This comment has been minimized.

Member

ddfreyne commented Dec 1, 2018

Documentation: nanoc/nanoc.ws/pull/228

@ddfreyne ddfreyne merged commit 6e56a14 into master Dec 1, 2018

3 checks passed

codecov/patch Coverage not affected when comparing 9cbd2ab...a91d8c0
Details
codecov/project 98.43% (+<.01%) compared to 9cbd2ab
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ddfreyne ddfreyne deleted the breadcrumbs-less-ambiguity branch Dec 1, 2018

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