Skip to content
This repository was archived by the owner on Sep 30, 2025. It is now read-only.

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

Merged
pbrisbin merged 8 commits into
pbrisbin:mainfrom
eahlberg:fix-widget-pagination-links
Jun 13, 2022
Merged

Do not filter out parameters with the same name in pagination links#41
pbrisbin merged 8 commits into
pbrisbin:mainfrom
eahlberg:fix-widget-pagination-links

Conversation

@eahlberg

Copy link
Copy Markdown
Contributor

Fixes #40

Comment thread src/Yesod/Paginator/Widgets.hs Outdated
Comment thread test/Yesod/Paginator/WidgetsSpec.hs Outdated
@eahlberg

eahlberg commented May 11, 2022

Copy link
Copy Markdown
Contributor Author

@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
Comment thread test/Yesod/Paginator/WidgetsSpec.hs Outdated
@eahlberg
eahlberg force-pushed the fix-widget-pagination-links branch from 57545fd to 085ec10 Compare May 11, 2022 15:01

@pbrisbin pbrisbin left a comment

Copy link
Copy Markdown
Owner

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
Copy Markdown
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
Comment thread yesod-paginator.cabal
Comment thread CHANGELOG.md
@eahlberg

Copy link
Copy Markdown
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.

@pbrisbin pbrisbin left a comment

Copy link
Copy Markdown
Owner

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.

Comment thread CHANGELOG.md
Comment thread CHANGELOG.md Outdated
Comment thread yesod-paginator.cabal
Comment thread test/Yesod/Paginator/WidgetsSpec.hs Outdated
Comment thread test/Yesod/Paginator/WidgetsSpec.hs Outdated
eahlberg and others added 2 commits May 12, 2022 10:17
Co-authored-by: patrick brisbin <pbrisbin@gmail.com>
@eahlberg
eahlberg force-pushed the fix-widget-pagination-links branch from 21d4606 to 7954604 Compare May 13, 2022 08:03
Comment thread src/Yesod/Paginator/Widgets.hs Outdated
Comment thread src/Yesod/Paginator/Widgets.hs Outdated
Comment thread test/Yesod/Paginator/WidgetsSpec.hs Outdated
@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
Comment thread test/SpecHelper.hs Outdated
Comment thread test/Yesod/Paginator/WidgetsSpec.hs Outdated
Comment thread test/Yesod/Paginator/WidgetsSpec.hs
@eahlberg

Copy link
Copy Markdown
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!

@pbrisbin pbrisbin left a comment

Copy link
Copy Markdown
Owner

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!

Comment thread test/Yesod/Paginator/WidgetsSpec.hs Outdated
Comment thread test/Yesod/Paginator/WidgetsSpec.hs
Comment thread test/Yesod/Paginator/WidgetsSpec.hs Outdated
@eahlberg

Copy link
Copy Markdown
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
Copy Markdown
Owner

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

@eahlberg

eahlberg commented Jun 9, 2022

Copy link
Copy Markdown
Contributor Author

@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

pbrisbin commented Jun 9, 2022

Copy link
Copy Markdown
Owner

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
Copy Markdown
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
Copy Markdown
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 subscribe to this conversation on GitHub. Already have an account? Sign in.

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