Skip to content

Conversation

@glyh
Copy link
Member

@glyh glyh commented May 26, 2025

EDIT: didn't expect this to break CI

@glyh glyh requested a review from a team as a code owner May 26, 2025 09:57
@glyh
Copy link
Member Author

glyh commented May 26, 2025

!ci-build-me

@dannywillems
Copy link
Member

!ci-bypass-changelog

Format.eprintf "A state hash should not be given for %s@."
ledger_kind ;
Core_kernel.exit 1 )
Core.exit 1 )
Copy link
Member

Choose a reason for hiding this comment

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

I would simply use Stdlib.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I did on iteration 1, but for consistency I didn't refer to Stdlib, but only Core

(Yojson.Safe.pretty_print ?std:None)
(Error_json.error_to_yojson err) ;
Core_kernel.exit 1 ) )
Core.exit 1 ) )
Copy link
Member

Choose a reason for hiding this comment

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

I would simply use Stdlib

Comment on lines 2300 to 2299
Core.print_endline signature_kind_string )
print_endline signature_kind_string )
Copy link
Member

@cjjdespres cjjdespres Jul 7, 2025

Choose a reason for hiding this comment

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

The reason why CI broke is that this is inside a Command.basic and not a Command.async, so I'm pretty sure the async scheduler is not active here. The existing Core.print_endline just prints to stdout, so it works. This print_endline is the async one and tries to add the write to the async queue, so nothing ends up being printed - I guess just silently does nothing when the async scheduler isn't active?

Copy link
Member

Choose a reason for hiding this comment

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

(The failing test was the one that makes sure that mina advanced print-signature-kind still works).

Copy link
Member Author

Choose a reason for hiding this comment

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

I can build with this though
image

Copy link
Member Author

Choose a reason for hiding this comment

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

But anyway it's bad practice to call sync printline in async code. Let me just make that async

@glyh glyh changed the title Init.Client: trivial clean up Init.Client: don't use both Core_kernel and Core Jul 8, 2025
@glyh
Copy link
Member Author

glyh commented Jul 8, 2025

Rebased this atop compatible: now it's taking forever to load nix on this branch

@glyh glyh force-pushed the corvo/clean-up-client branch from 161fdad to 7ff5c42 Compare July 8, 2025 02:04
@glyh glyh added the enhancement Not big enough to be a feature, but is a smaller improvement label Jul 8, 2025
@glyh
Copy link
Member Author

glyh commented Jul 8, 2025

!ci-build-me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Not big enough to be a feature, but is a smaller improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants