-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[CLI] Add message-format-json
experiment value to aptos move compile
and aptos move lint
#15540
base: main
Are you sure you want to change the base?
Conversation
⏱️ 11s total CI duration on this PR
|
/// Run move compiler and print errors to given writer. Returns the set of compiled units. | ||
pub fn run_move_compiler_with_emitter( | ||
mut emitter: Box<dyn Emitter>, | ||
options: Options, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the code duplication? Could we not create the emitter based on the message format in options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, it's a draft, I'll remove it when someone confirms that the Emitter trait approach works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -107,6 +110,8 @@ pub struct BuildOptions { | |||
/// Select bytecode, language, compiler for Move 2 | |||
#[clap(long)] | |||
pub move_2: bool, | |||
#[clap(long, default_value = "human")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of another field, could we just reuse experiments
above, and have this be another "experiment" (that is, if the experiment is on, then we have JSON formatted messages, e.g.)? See third_party/move/move-compiler-v2/src/experiments.rs
and the function experiments_from_opt_level
for an example of threading through an "option" via experiments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it's a better option. I'll rewrite the PR now, removing the message_format
field, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
--message-format
to aptos move compile
and aptos move lint
message-format-json
experiment value to aptos move compile
and aptos move lint
Description
I would like to add the flag to the CLI which changes all the human-readable codespan_reporting output into the JSON output.
I specifically need it for the proper integration with the IDE.
How Has This Been Tested?
Not tested yet.
Key Areas to Review
This is WIP. I have only added the flag for the compilation and lint errors. I also ignored compiler-v1, I assume it's eventually going to be removed, so it's alright to just add no-op flag there.
@vgao1996 Could you look at this impl and let me know whether the approach is acceptable?
Type of Change
Which Components or Systems Does This Change Impact?
Checklist