Convert Json module from String to ByteString #6948
Conversation
|
cc @phadej |
Cabal/Distribution/Utils/Json.hs
Outdated
| | JsonNull | ||
| | JsonNumber !Int | ||
| | JsonObject [(ByteString, Json)] | ||
| | JsonString !ByteString |
There was a problem hiding this comment.
What kind of strings are used here. I'd use either ShortText or String inside Json type.
E.g. package-ids, unit-ids are backed up by ShortText (and they are in UTF8, so if they don't need escaping they can be used as is).
Yet, currently filepaths are FilePath i.e. String.
Here are two issues:
- The keys and strings are textual
- double conversions. Thus I'd use
ShortTextfor keys inJsonObject(as these are most likely static), and would useStringforJsonString(as it looks like most producers areprettyShowor alike).- I'd like it to be
ShortText, but if most producers atm are producingString, that would be silly. Yet, there could be a comment to eventually change it toShortText(Path abstraction is related: Introduce Path abstraction #6667)
- I'd like it to be
There was a problem hiding this comment.
Is it really a double conversion if this all gets converted to a ByteString at the end of the day? As far as I can see for JsonString, all the filepaths etc. will go from String -> ByteString, and then once it's in a ByteString the builder means it will be cheap to insert it and move it around
lukel97
force-pushed
the
json-bytestring
branch
2 times, most recently
from
d4dbd48 to
0fe8abf
Compare
lukel97
force-pushed
the
json-bytestring
branch
2 times, most recently
from
bc9558e to
7e92f25
Compare
| @@ -349,7 +349,7 @@ library | |||
|
|
|||
| 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 | |||
There was a problem hiding this 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 :)
There was a problem hiding this comment.
Don't we need a newer bytestring on an older GHC here though?
|
I'll postpone merging this after I cut the 3.4 branch, which I hopefully will manage to do today. |
|
Failing build looks flakey |
|
Now that 3.4 has been released, what's the status of this PR? |
|
uh the tag is in this repo but the download page has not been updated? |
|
It may be because cabal-install 3.4 is still unreleased |
|
oops sorry even tag is for Cabal and no cabal-install |
|
@bubba could you rebase? |
|
Closing in favor of the rebased pr. |
Taken from #6241