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

Do not filter out parameters with the same name in pagination links #41

Merged
merged 8 commits into from
Jun 13, 2022

Conversation

eahlberg
Copy link
Contributor

Fixes #40

@eahlberg
Copy link
Contributor Author

eahlberg commented May 11, 2022

@pbrisbin feel free to take a look at this when time permits. I also left a few comments above. FYI: I'm on GHC 8.10.7 and couldn't test this using GHC 9.0.2, if this causes any issues I'll try to get 9.0.2 up and running.

@eahlberg eahlberg marked this pull request as ready for review May 11, 2022 14:51
@eahlberg eahlberg force-pushed the fix-widget-pagination-links branch from 57545fd to 085ec10 Compare May 11, 2022 15:01
Copy link
Owner

@pbrisbin pbrisbin left a comment

Choose a reason for hiding this comment

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

This is great, thank you.

@pbrisbin
Copy link
Owner

Can you go ahead and bump the minor version in package.yaml (and don't forget to also commit the .cabal change that results) and update the CHANGELOG? That way, when I merge this, it'll auto-release to Hackage.

@eahlberg eahlberg force-pushed the fix-widget-pagination-links branch from 78c829a to 8ab6082 Compare May 11, 2022 17:50
CHANGELOG.md Show resolved Hide resolved
@eahlberg
Copy link
Contributor Author

@pbrisbin I went with the approach of exporting the parameter filtering function to simplify the spec and left a few comments above regarding things I'm unsure of, it'd be nice if you could take a look.

Copy link
Owner

@pbrisbin pbrisbin left a comment

Choose a reason for hiding this comment

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

Thanks for your continued efforts here, I requested some minor changes.

CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
yesod-paginator.cabal Show resolved Hide resolved
test/Yesod/Paginator/WidgetsSpec.hs Outdated Show resolved Hide resolved
test/Yesod/Paginator/WidgetsSpec.hs Outdated Show resolved Hide resolved
@eahlberg eahlberg force-pushed the fix-widget-pagination-links branch from 21d4606 to 7954604 Compare May 13, 2022 08:03
src/Yesod/Paginator/Widgets.hs Outdated Show resolved Hide resolved
test/Yesod/Paginator/WidgetsSpec.hs Outdated Show resolved Hide resolved
@eahlberg eahlberg force-pushed the fix-widget-pagination-links branch 4 times, most recently from 9b29f4c to 42c4958 Compare May 30, 2022 13:13
@eahlberg eahlberg force-pushed the fix-widget-pagination-links branch from 42c4958 to 4537442 Compare May 30, 2022 13:15
test/SpecHelper.hs Outdated Show resolved Hide resolved
@eahlberg
Copy link
Contributor Author

@pbrisbin I took another look at this and tried to clean up the conversation above a bit. I couldn't come up with property tests for everything but added a couple of property tests, unit tests and left a few comments. Not sure if the CI failure is related to this.

Feel free to take a look when time permits and let me know if I've missed anything or if you have any questions!

Copy link
Owner

@pbrisbin pbrisbin left a comment

Choose a reason for hiding this comment

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

This looks good overall, thanks! There are just a few organizational things I noted, but I'm willing to just fix that up myself and push to this PR before merging, if you prefer. Let me know!

test/Yesod/Paginator/WidgetsSpec.hs Outdated Show resolved Hide resolved
test/Yesod/Paginator/WidgetsSpec.hs Show resolved Hide resolved
test/Yesod/Paginator/WidgetsSpec.hs Outdated Show resolved Hide resolved
@eahlberg
Copy link
Contributor Author

@pbrisbin thanks for the feedback, I think all issues should be addressed!

@pbrisbin pbrisbin enabled auto-merge (rebase) May 31, 2022 14:37
@pbrisbin
Copy link
Owner

Awesome. I set this to auto-merge on green, which will auto-release to Hackage. Thanks!

@eahlberg
Copy link
Contributor Author

eahlberg commented Jun 9, 2022

@pbrisbin there are two CI jobs failing which I'm not sure if they're relevant or not: CI / test (stack.yaml) and restyled. If you have the time to take a look that'd be great.

@pbrisbin
Copy link
Owner

pbrisbin commented Jun 9, 2022

which I'm not sure if they're relevant or not

Just for future reference: if the PR check is Required, it's, well, required. Otherwise, it's not.

So let's ignore Restyled for now and look at the main test. It looks like a new weeder was released and the latest version doesn't work with current LTS. That project runs ahead and this happens a lot. We just have to tell the action to use a version that works. I think this should do it,

diff --git i/.github/workflows/ci.yml w/.github/workflows/ci.yml
index 95effd4..58b08c8 100644
--- i/.github/workflows/ci.yml
+++ w/.github/workflows/ci.yml
@@ -47,6 +47,8 @@ jobs:

       - if: ${{ matrix.stack-yaml == 'stack.yaml' }}
         uses: freckle/weeder-action@v1
+        with:
+          weeder-version: 2.3.0

   hlint:
     runs-on: ubuntu-latest

auto-merge was automatically disabled June 13, 2022 08:22

Head branch was pushed to by a user without write access

@eahlberg
Copy link
Contributor Author

Missed the Required tag completely in the UI, thanks for clarifying. I updated the Weeder version as you suggested but there were some jobs which immediately failed due to GitHub Actions being unavailable, perhaps you could trigger a re-run when time permits and see if it is back up.

@pbrisbin
Copy link
Owner

Wow yeah, still down. I'll keep an eye on the status page and re-run when it's back up.

@pbrisbin pbrisbin enabled auto-merge (squash) June 13, 2022 14:41
@pbrisbin pbrisbin merged commit 3480159 into pbrisbin:main Jun 13, 2022
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.

Pagination widgets filter out parameters with the same name
2 participants