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

Add appendPageQueryParam parameter #8

Merged
merged 4 commits into from
May 17, 2020
Merged

Add appendPageQueryParam parameter #8

merged 4 commits into from
May 17, 2020

Conversation

benbristow
Copy link
Contributor

@benbristow benbristow commented Apr 29, 2020

Adds an option on each SitemapNode to disable the concatenation of the ?page= param to each loc

@PureKrome PureKrome self-requested a review April 29, 2020 22:41
Copy link
Owner

@PureKrome PureKrome left a comment

Choose a reason for hiding this comment

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

Wow nice stuff! really appreciate the PR @benbristow ! 👏

Adds an option on each SitemapNode to disable the concatenation of the ?page= param to each loc

Now I understand the code .. but I'm missing one piece of info: what's the scenario where you don't want ?page=<...> ? Is that if you have only one page?

.gitignore Outdated
*.orig
Copy link
Owner

Choose a reason for hiding this comment

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

What are these two files, for?

Copy link
Contributor Author

@benbristow benbristow Apr 30, 2020

Choose a reason for hiding this comment

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

No idea, but that was there before.

Copy link
Owner

Choose a reason for hiding this comment

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

ha oh yeah. it's a weird 'changed' line.

Copy link
Owner

Choose a reason for hiding this comment

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

oh wait. i ment -> what is an .idea/ folder? that's something u added.

Copy link
Owner

Choose a reason for hiding this comment

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

AH! is this related to JETBRAINS IntelliJ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the slow reply, been swamped with work.

Oh, it might have been. I've been using JetBrains Rider (Visual Studio alternative). I can remove if required?

Copy link
Owner

Choose a reason for hiding this comment

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

yes please @benbristow - then we can merge this PR. 👍

Copy link
Owner

Choose a reason for hiding this comment

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

👋 very polite ping :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, never received this ping. Updated.

src/SimpleSitemap/SitemapService.cs Show resolved Hide resolved
@benbristow
Copy link
Contributor Author

Wow nice stuff! really appreciate the PR @benbristow ! 👏

Adds an option on each SitemapNode to disable the concatenation of the ?page= param to each loc

Now I understand the code .. but I'm missing one piece of info: what's the scenario where you don't want ?page=<...> ? Is that if you have only one page?

Mainly if you've got a ?page= query param already there for pagination (or other query params there as it'd look a bit weird like mypage/?foo=bar?page=2

I've never seen a sitemap with the ?page= query param before added to the loc entries.

Copy link
Owner

@PureKrome PureKrome left a comment

Choose a reason for hiding this comment

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

Ok - nearly there. Just that last question about the .idea file. Can you confirm if that's for IntelliJ?

.gitignore Outdated
*.orig
Copy link
Owner

Choose a reason for hiding this comment

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

AH! is this related to JETBRAINS IntelliJ ?

src/SimpleSitemap/SitemapService.cs Show resolved Hide resolved
@benbristow
Copy link
Contributor Author

benbristow commented May 17, 2020

I see the tests have failed. I can't seem to reproduce this locally after re-running in Visual Studio many times (10+). I think this is a blip on Appveyor's side, especially as I only updated the .gitignore file. Are you able to re-run the tests on Appveyor?

@PureKrome
Copy link
Owner

Error seems related to a missing file. I'll try and pull down the branch and test that locally, later.

watch this space!

@codecov
Copy link

codecov bot commented May 17, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@c259038). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master       #8   +/-   ##
=========================================
  Coverage          ?   89.68%           
=========================================
  Files             ?        3           
  Lines             ?      126           
  Branches          ?        0           
=========================================
  Hits              ?      113           
  Misses            ?       13           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c259038...83e12a2. Read the comment docs.

@PureKrome PureKrome merged commit 81d52a3 into PureKrome:master May 17, 2020
@PureKrome
Copy link
Owner

Done @benbristow :) I fixed the issue (ref 83e12a2) and merged.

NuGet has also been updated. It's now version 3.1.0.

Thanks heaps for the PR - really appreciate it.

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.

2 participants