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.

Sign up for GitHub

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

Jump to bottom

Exported Servant.Links.addQueryParam #1785

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Exported Servant.Links.addQueryParam #1785

wants to merge 1 commit into from

Conversation

saurabhnanda
Copy link

Closes #1232

tchoutri
tchoutri approved these changes Sep 23, 2024
View reviewed changes
servant/src/Servant/Links.hs
Comment on lines +193 to +195
-- NOTE: If you are writing a custom 'HasLink' instance, and need to manipulate
-- the 'Link' (adding query params or fragments, perhaps), please use the the
-- 'addQueryParam' and 'addSegment' functions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Good Note, thanks!

Copy link
Author

Choose a reason for hiding this comment

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

@tchoutri should we also export addSegment? Which would force one to also export Fragment'? Is there any reason why one would be extending / modifying this behaviour?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not in this PR, I would like to take the time to think about a different module to export these "unsafe" modifiers for Links.

@tchoutri
Copy link
Contributor

@saurabhnanda Thanks a lot for this!

Could you please add a new changelog entry in the changelog.d file? Just like this one https://github.com/haskell-servant/servant/blob/master/changelog.d/issue-1780

ysangkok
ysangkok reviewed Sep 23, 2024
View reviewed changes
servant/servant.cabal Outdated Show resolved Hide resolved
@saurabhnanda
Copy link
Author

@saurabhnanda Thanks a lot for this!

Could you please add a new changelog entry in the changelog.d file? Just like this one https://github.com/haskell-servant/servant/blob/master/changelog.d/issue-1780

@tchoutri I had created a branch from v0.20.2 and not master. I believe changelog.d has been introduced in master, along with some GHC-related deprecations? Are those intended? Because I am unable to build servant on Stack LTS-18.28 if I swtich to master branch.

       
       In the dependencies for servant-0.20.3:
         * base must match >=4.16.4.0 && <4.21, but base-4.14.3.0 is in the Stack configuration (latest matching version is 4.20.0.1).
         * bytestring must match >=0.11 && <0.13, but bytestring-0.10.12.0 is in the Stack configuration (latest matching version is 0.12.1.0).
       The above is/are needed since servant is a build target.

@tchoutri
Copy link
Contributor

@saurabhnanda changelog-d has been around for quite some time, if I'm not mistaken.

Regarding the GHC deprecations: yes they are intended, see #1778.

You will need to update the Stack setup to use the LTS of 9.6.6.

@saurabhnanda
Copy link
Author

@tchoutri added the changelog entry - hope it's fine.

tchoutri
tchoutri reviewed Sep 24, 2024
View reviewed changes
changelog.d/issue-1232
packages: servant
prs: #1785
issues: #1232
significance: minor
Copy link
Contributor

Choose a reason for hiding this comment

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

@fgaz remind me, is minor a valid value for the significance field?

Copy link

Choose a reason for hiding this comment

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

No, we only have significance: significant or omitted

Copy link
Contributor

Choose a reason for hiding this comment

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

@tchoutri btw the docs are here: https://codeberg.org/fgaz/changelog-d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tchoutri tchoutri tchoutri approved these changes

@fgaz fgaz fgaz left review comments

@ysangkok ysangkok ysangkok left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Servant.Links.addQueryParam not exported?
4 participants
@saurabhnanda @tchoutri @ysangkok @fgaz