Skip to content

Conversation

@marinelli
Copy link
Contributor

@marinelli marinelli commented Mar 1, 2022

This PR proposes to:

✅ Checklist for your Pull Request

Related changes (conditional)

  • Tests

    • If I added new functionality, I added tests covering it.
    • If I fixed a bug, I added a regression test to prevent the bug from
      silently reappearing again.
  • Documentation

    • I checked whether I should update the docs and did so if necessary:
  • Public contracts

    • Any modifications of public contracts comply with the Evolution
      of Public Contracts
      policy.
    • I added an entry to the changelog if my changes are visible to the users
      and
    • provided a migration guide for breaking changes if possible.

Stylistic guide (mandatory)

  • Style
    • My commits comply with the policy used in Serokell.
    • My code complies with the style guide.
    • Each commit of this pull request contains a description.
      • For new features, this description contains the use case for the new functionality.
      • For bug fixes, it describes both the attacked problem and a solution for it.
      • For dependency version changes description shortly enlists features and breaking changes of the new library version or contains a link to its changelog.

type EncodedQueryParam = Text
encodeQueryParam :: ToHttpApiData a => a -> EncodedQueryParam
encodeQueryParam = toQueryParam
#endif
Copy link

Choose a reason for hiding this comment

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

This seems really awkward. How bad would it be to bump the dependency version?

Copy link
Contributor Author

@marinelli marinelli Mar 1, 2022

Choose a reason for hiding this comment

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

I do not like this solution too. If supporting servant 0.18 is not required, let's remove it.
Btw, I'll create an other PR to export encodeQueryParam from the servant-client-core package.
Since this haskell-servant/servant#1549, it would not be required anymore to add the encodeQueryParam function, it might be used the encodeQueryParamValue function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you were thinking something like this marinelli@a0dbeeb?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Martoon-00 what do you think about it?

Copy link
Member

Choose a reason for hiding this comment

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

I apologize for the late reply 🙏 🙏

I don't like such tricks with CPP pragmas too, but in this case its scope is sufficiently limited and it seems like the least evil to me.

Let's go with the current solution to leave others some time if they need a recent feature of servant-util but cannot afford to switch to the servant-0.19. And after a couple of major releases of servant, or if this CPP turns out to hinder other features, we will strip the old behaviour.

I also asked our OPS team to make CI run both the old and new versions of servant-client-core (internal ticket for this).


@marinelli You are also very welcome to start using haskell-servant/servant#1549 here as soon as it appears in a release. Just leave a note in the changelog that we do not support servant-0.19, rather only < 0.19 and >= 0.19.X.

Thanks a lot for your work!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure anymore that re-exporting encodeQueryParamValue from servant-client-core is a good idea :)
We'll see.

@marinelli marinelli marked this pull request as ready for review March 1, 2022 10:29
type EncodedQueryParam = Text
encodeQueryParam :: ToHttpApiData a => a -> EncodedQueryParam
encodeQueryParam = toQueryParam
#endif
Copy link
Member

Choose a reason for hiding this comment

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

I apologize for the late reply 🙏 🙏

I don't like such tricks with CPP pragmas too, but in this case its scope is sufficiently limited and it seems like the least evil to me.

Let's go with the current solution to leave others some time if they need a recent feature of servant-util but cannot afford to switch to the servant-0.19. And after a couple of major releases of servant, or if this CPP turns out to hinder other features, we will strip the old behaviour.

I also asked our OPS team to make CI run both the old and new versions of servant-client-core (internal ticket for this).


@marinelli You are also very welcome to start using haskell-servant/servant#1549 here as soon as it appears in a release. Just leave a note in the changelog that we do not support servant-0.19, rather only < 0.19 and >= 0.19.X.

Thanks a lot for your work!

Copy link
Member

@Martoon-00 Martoon-00 left a comment

Choose a reason for hiding this comment

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

Again thank you for the update.

Please rebase and let's merge it.

@marinelli marinelli requested a review from treeowl May 20, 2022 06:31
@marinelli
Copy link
Contributor Author

Again thank you for the update.

Please rebase and let's merge it.

I rebased and updated the servant-util/CHANGES.md file.

@marinelli marinelli requested a review from Martoon-00 May 20, 2022 06:49
@Martoon-00 Martoon-00 merged commit 29d3088 into serokell:master May 20, 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.

Build problem with servant 0.19

3 participants