Skip to content

further reduce newlines for compact output#110

Merged
georgefst merged 4 commits intocdepillabout:masterfrom
juhp:patch-1
Jun 4, 2022
Merged

further reduce newlines for compact output#110
georgefst merged 4 commits intocdepillabout:masterfrom
juhp:patch-1

Conversation

@juhp
Copy link
Copy Markdown
Contributor

@juhp juhp commented May 31, 2022

This seems to rather make the compact output have less newline commas: addressing #84.

eg

    ( "request"
    , ValueArray
        [ ValueString "git+https://src.fedoraproject.org/rpms/rpmbuild-order.git#6a9f3fce18040a2020d37707abb82445e1884e69"
        , ValueString "f37-build-side-54239"
        , ValueStruct
            [
                ( "fail_fast", ValueBool True )
            ,
                ( "wait_builds", ValueArray [] )
            ,
                ( "custom_user_metadata", ValueStruct [] ) ] ] )
,
    ( "result"
    , ValueStruct
        [
            ( "faultCode", ValueInt 1000 )
        ,
            ( "faultString"
            , ValueString "bad filename: A-1-1.fc37.buildreqs.nosrc.rpm (expected A-1-1.fc37.src.rpm)" ) ] )
,
    ( "start_time", ValueString "2022-05-31 02:27:37.208059+00:00" )

becomes

    ( "request", ValueArray
        [ ValueString "git+https://src.fedoraproject.org/rpms/rpmbuild-order.git#6a9f3fce18040a2020d37707abb82445e1884e69", ValueString "f37-build-side-54239", ValueStruct
            [
                ( "fail_fast", ValueBool True ),
                ( "wait_builds", ValueArray [] ),
                ( "custom_user_metadata", ValueStruct [] ) ] ] ),
    ( "result", ValueStruct
        [
            ( "faultCode", ValueInt 1000 ),
            ( "faultString", ValueString "bad filename: A-1-1.fc37.buildreqs.nosrc.rpm (expected A-1-1.fc37.src.rpm)" ) ] ),
    ( "start_time", ValueString "2022-05-31 02:27:37.208059+00:00" ),

@cdepillabout
Copy link
Copy Markdown
Owner

cdepillabout commented Jun 3, 2022

I don't generally use the compact output option, so I don't have a strong opinion on this change, but it does roughly look to me like what someone who wants more compact output would want.

@georgefst How does this look to you?

@juhp I'm wondering if any of these doctests will need to get fixed?

pretty-simple/src/Text/Pretty/Simple.hs

Line 663 in abbfd69

-- >>> pPrintStringOpt CheckColorTty defaultOutputOptionsDarkBg {outputOptionsCompact = True} "AST [] [Def ((3,1),(5,30)) (Id \"fact'\" \"fact'\") [] (Forall ((3,9),(3,26)) [((Id \"n\" \"n_0\"),KPromote (TyCon (Id \"Nat\" \"Nat\")))])]"
. If none of these need fixing (which is somewhat surprising), could you add a new doctest that would fail on master, but would now pass with this PR?

@juhp
Copy link
Copy Markdown
Contributor Author

juhp commented Jun 4, 2022 •
edited
Loading

Yea, I was a little surprised too: thanks for pointing out the doctests.

I have a added a simple doctest which is affected by this change.

Previously the output would have been:

          [
              ( "id", 123 )
          ,
              ( "state", 1 )
          ,
              ( "pass", 1 )
          ,
              ( "tested", 100 )
          ,
              ( "time", 12345 )
          ]

@georgefst
Copy link
Copy Markdown
Collaborator

georgefst commented Jun 4, 2022

Nice! As I've said elsewhere, compact mode was kind of a quick hacky afterthought. This makes it a bit more useful and intuitive.

I hope you don't mind that I've pushed two small refactors.

georgefst
georgefst reviewed Jun 4, 2022
View reviewed changes
@georgefst georgefst mentioned this pull request Jun 4, 2022
Open
@georgefst
Copy link
Copy Markdown
Collaborator

georgefst commented Jun 4, 2022

RE #84 (comment): would you be interested in also helping to improve the output of records? We can tackle that separately otherwise.

@juhp
Copy link
Copy Markdown
Contributor Author

juhp commented Jun 4, 2022

RE #84 (comment): would you be interested in also helping to improve the output of records?

I think I would prefer it to be tackled separately - perhaps I can have a look at some point.

@georgefst
Copy link
Copy Markdown
Collaborator

georgefst commented Jun 4, 2022

I think I would prefer it to be tackled separately - perhaps I can have a look at some point.

Ok, no worries!

@georgefst georgefst merged commit da22de6 into cdepillabout:master Jun 4, 2022
@juhp juhp deleted the patch-1 branch June 5, 2022 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@georgefst georgefst georgefst 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.

3 participants

@juhp @cdepillabout @georgefst