Skip to content

Json bytestring#7477

Merged
emilypi merged 3 commits intohaskell:masterfrom
fendor:json-bytestring
Aug 15, 2021
Merged

Json bytestring#7477
emilypi merged 3 commits intohaskell:masterfrom
fendor:json-bytestring

Conversation

@fendor
Copy link
Collaborator

@fendor fendor commented Jul 8, 2021

@fendor fendor force-pushed the json-bytestring branch 2 times, most recently from 3c480f7 to eed2566 Compare July 8, 2021 13:14
@Mikolaj Mikolaj self-requested a review July 8, 2021 13:36
Comment on lines +27 to +30
renderJson (JsonObject
[ ("key", JsonString "foo\"bar")
, ("key2", JsonString "baz")])
@?= "{\"key\":\"foo\\\"bar\",\"key2\":\"baz\"}"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Mikolaj, @emilypi Do we have some form of formatting standards for new code?

Copy link
Member

Choose a reason for hiding this comment

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

There's been no concrete discussion of standards. Don't worry about it for now, and we'll come up with one

Copy link
Member

Choose a reason for hiding this comment

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

Right. We only have https://github.com/haskell/cabal/blob/master/CONTRIBUTING.md#conventions, as mentioned in PR template. I guess, for now, use whatever conventions exist in the snippet you edit, unless they are utterly insane (in which case, create a reformatting commit first).

if !impl(ghc >= 7.8)
-- semigroups depends on tagged.
build-depends: tagged >=0.8.6 && <0.9
build-depends: tagged >=0.8.6 && <0.9, bytestring-builder >= 0.10.8 && <0.11
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From #6948 (comment)

This is wrong as is, but I'll fix it before merge. One can have older bytestring on newer GHC, though that is contrived corner-case. But Cabal should be exemplary in own definition :)

Does that mean we don't need this dependency? Ill try to remove it and see whether CI is green.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I guess CI tells us that it is not fine. I still don't know why it is wrong though. @phadej Would you mind elaborating?

Copy link
Member

Choose a reason for hiding this comment

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

@phadej: ping?

@fendor fendor force-pushed the json-bytestring branch 2 times, most recently from ea01556 to 4c34d78 Compare July 14, 2021 06:58
@fendor
Copy link
Collaborator Author

fendor commented Jul 14, 2021

Theoretically ready to merge, but we need to resolve #7477 (comment) before merging.

@fendor
Copy link
Collaborator Author

fendor commented Aug 15, 2021

@emilypi, @Mikolaj This PR is waiting for a decision in #7477 (comment), do you have any insights, or should be just merge?

@emilypi
Copy link
Member

emilypi commented Aug 15, 2021

@fendor the edge case seems like it'd go away as we drop GHC's in the next series of PRs. I say let's merge.

Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

thanks!

@emilypi emilypi merged commit fd185f3 into haskell:master Aug 15, 2021
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.

5 participants