-
Notifications
You must be signed in to change notification settings - Fork 340
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
Interprets excludedPaths as path prefixes in the SiteMap generator #65
Interprets excludedPaths as path prefixes in the SiteMap generator #65
Conversation
|
||
import Foundation | ||
|
||
internal extension Collection where Element: StringWrapper { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is an unnecessary abstraction, given that it's only being used in a single place.
|
||
internal extension Collection where Element: StringWrapper { | ||
func containsPrefixFor(_ string: String) -> Bool { | ||
reduce(false){ $0 || string.hasPrefix($1.string) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will needlessly iterate through the collection every time, even if a match was found, making us turn an O(1)
operation (checking if a set contains a member) into an operation that always is O(N)
(not only in the worst case). I would suggest using the contains(where:)
API instead:
contains(where: { $0.string.hasPrefix(string) })
@@ -27,11 +27,17 @@ struct SiteMapGenerator<Site: Website> { | |||
} | |||
} | |||
|
|||
internal extension Collection where Element: StringWrapper { | |||
func containsPath(_ path: Path) -> Bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the extension you added in a new file could simply be inlined here.
Thanks for reviewing, @JohnSundell! I finally got around to updating this PR and adding some test cases to verify the new behavior. Take a look and let me know what you think! |
Spend all night trying to handle this problem with the API, then I saw this. I need this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @bensyverson!
Summary
Currently, the
SiteMapGenerator
uses exact matches when comparing apath
to exclude from the SiteMap.This PR changes that behavior, such that the
excludedPaths
are interpreted as excluded path prefixes.Discussion
Real world example: I have a Publish site which has a Photography
Section
, with severalItem
s in a subfolder:I'd like the SiteMap to include
/photography/index.html
but not any of the/photography/photos/
pages. In Publish as it stands, that's impossible, because it's doing an exact match on the path for eachItem
:This PR changes the matching behavior so that the
excludedPaths
are treated as excluded prefixes for a given path. So if you pass in:"photography/photos/"
it will block all of theItem
s which share that prefix.Implications & Alternatives
An advantage of this approach is that you can exclude more than one branch or
Section
with a single prefix;"pho"
will exclude/photography
as well as/phone
.The obvious "gotcha" is that this could have unintended consequences; if you have both
foo
andfood
sections, then excludingfoo
will exclude all offood
too. One solution is to include the trailing slash;foo/
, however that will only blockfoo
's items, not its index.Alternative: wildcards
One alternative would be to support wildcard syntax, so that you could pass in
photography/*
orfoo*
to explicitly "opt-in" to prefix matching. While totally reasonable, this may be overkill?Thanks again John for your very thoughtful work. Sorry for all the PRs—these are just the nips and tucks I'm finding as I dig into the frameworks!