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

Add skeletal format subcommand. #4383

Merged
merged 2 commits into from
Oct 9, 2024
Merged

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Oct 8, 2024

This extracts out the SourceBuffer handling of - in order to trivially share it.

Note this still has a number of TODOs, it's just setting up the essential subcommand infrastructure, with some tests demonstrating that it at least does something.

@jonmeow
Copy link
Contributor Author

jonmeow commented Oct 8, 2024

Part 1 of 4 (each using the last/eponymous commit)

1: #4383
2: #4384
3: #4385
4: #4386

Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

Generally LG, some fairly minor comments inline (TODOs, a naming tweak).

Feel free to land with fixes.

return result;
}

auto per_file_error = [&]() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe:

Suggested change
auto per_file_error = [&]() {
auto mark_per_file_error = [&]() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

result.per_file_success.push_back({f.str(), true});

// TODO: Consider refactoring this for sharing with compile.
auto source = SourceBuffer::MakeFromFileOrStdin(driver_env.fs, f, consumer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to check for - here when there are multiple inputs? Or does the driver environment already handle that?

Copy link
Contributor

Choose a reason for hiding this comment

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

(or a TODO, I understand this is just a framework)

Copy link
Contributor Author

@jonmeow jonmeow Oct 9, 2024

Choose a reason for hiding this comment

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

Added a TODO. I grant the behavior for the user if they specify - multiple times may be odd (content in the first, empty for later, I'm expecting).

I think the driver environment doesn't handle this. Maybe it should, given that compile would have that issue too? Might mean factoring out this function wasn't worthwhile on the whole.

Comment on lines +14 to +16
// Options for the format subcommand.
//
// See the implementation of `Build` for documentation on members.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a TODO here or on the Build command for adding line-range flags and formatting support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm putting this on the Format call... later PRs are putting a few TODOs there already, so it seems like a good central spot to find missing features.

@jonmeow jonmeow enabled auto-merge October 9, 2024 16:56
@jonmeow jonmeow added this pull request to the merge queue Oct 9, 2024
Merged via the queue into carbon-language:trunk with commit 434173b Oct 9, 2024
8 checks passed
@jonmeow jonmeow deleted the format branch October 9, 2024 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants