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.

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

further reduce newlines for compact output #110

Merged
merged 4 commits into from
Jun 4, 2022

Conversation

juhp
Copy link
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
Owner

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?

-- >>> 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
Contributor Author

juhp commented Jun 4, 2022

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
Collaborator

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
Copy link
Collaborator

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
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
Collaborator

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants