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

ensure host ends in a slash #245

Closed
wants to merge 2 commits into from
Closed

ensure host ends in a slash #245

wants to merge 2 commits into from

Conversation

jphager2
Copy link

Also updated specs that check equality of host or host + path to expect
trailing slash for host.

Also updated specs that check equality of host or host + path to expect
trailing slash for host.
@jphager2
Copy link
Author

This is in reference to #236.

@jphager2
Copy link
Author

@kjvarga anything I should do for this pull request?

raise SitemapGenerator::SitemapError, "No value set for #{key}" unless self[key]
end

def assert_slash(key)
Copy link
Owner

Choose a reason for hiding this comment

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

Please rename append_slash

@kjvarga
Copy link
Owner

kjvarga commented Aug 10, 2016

Sorry to take so long. I've commented.

@jphager2
Copy link
Author

Hi @kjvarga, I can revert the changes to the specs, but the problem will be that all specs that test #host will return host with a slash at the end. Which means that many of the specs will fail their expectation.

@jphager2
Copy link
Author

So any further thoughts on this, @kjvarga?

@kjvarga
Copy link
Owner

kjvarga commented Sep 23, 2016

I came back to this. I wasn't happy with modifying the host, and thought to rather just ensure a slash is present when the url is constructed. But I can't reproduce an error. The sitemaps_path portion will only be missing from the URI.join if it's present and doesn't end in a slash, but code already exists to ensure it ends in a slash.

Are you able to provide me an example where it fails? I can't reproduce.

@jphager2
Copy link
Author

See the comment here. #236

@jphager2 jphager2 closed this Sep 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants