Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 34 additions & 2 deletions src/formatting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,24 @@ impl<'a> Formatter<'a> {
self.write_expr_span(expr);
}

Expr::Signature(sig) => self.format_signature(sig),
Expr::Signature(sig) => {
let has_comments = self
.comments
.iter()
.any(|(span, _)| span.start >= expr.span.start && span.end <= expr.span.end);
if has_comments {
self.write_expr_span(expr);
Copy link

Choose a reason for hiding this comment

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

Does this mean there won't be any change in the signature if there's a doc comment?

Just out of curiosity from eyeballing the code as I don't have a machine with me right now to test, I think custom completion expressions in signatures are missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya, it should stay the same if there are comments. I'm not sure if that's right or wrong.

This code is really just thrown together to try and get over the hump of no one working on it. I really needs like 1000 samples for the ground truth. The intent was to be able to create and input script and an expected script and have it run those without compilation. However, it's all hard coded right now. The more samples we can get with different syntaxes the better testing will be and the better formatting will be.

Copy link

Choose a reason for hiding this comment

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

Glad to see such great improvement in nufmt. It feels almost there that I can archive topiary-nushell and let go, I thought it was an impossible task before a mature new-nu-parser.

On the other hand, the signature thing is till annoying, and I'm afraid too much hack on nufmt doesn't worth the effort for issues that should be originally addressed on the parser side.

Having said that, would you like me to report new issues, like the mentioned "custom completion expression in the signature", or do you have any other plan of how we should proceed with this project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya, please report issues. I may not fix them all. Some, like you say, are parser bugs. I definitely don't want you to archive topiary-nushell but I do want to prove (mostly to myself) that nufmt can actually work as a real-world formatter. I'm honestly not convinced right now. I still think i'll eat more code than it will format, but at least someone is trying to make it better.

Copy link

Choose a reason for hiding this comment

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

Right now, nufmt seems promising. And I think a single working and configurable formatter is enough for the community. topiary has its own limit that won't make everybody happy. If we can overcome some more obstacles either by improving nu-parser/new-nu-parser or nufmt, then I think topiary-nushell has fullfilled its task in history.

self.last_pos = expr.span.end;
// Mark comments within the span as written
for (i, (span, _)) in self.comments.iter().enumerate() {
if span.start >= expr.span.start && span.end <= expr.span.end {
self.written_comments[i] = true;
}
}
} else {
self.format_signature(sig);
}
}

Expr::Call(call) => self.format_call(call),
Expr::ExternalCall(head, args) => self.format_external_call(head, args),
Expand Down Expand Up @@ -443,7 +460,22 @@ impl<'a> Formatter<'a> {
fn format_def_argument(&mut self, positional: &Expression) {
match &positional.expr {
Expr::String(_) => self.format_expression(positional),
Expr::Signature(sig) => self.format_signature(sig),
Expr::Signature(sig) => {
let has_comments = self.comments.iter().any(|(span, _)| {
span.start >= positional.span.start && span.end <= positional.span.end
});
if has_comments {
self.write_expr_span(positional);
// Mark comments within the span as written
for (i, (span, _)) in self.comments.iter().enumerate() {
if span.start >= positional.span.start && span.end <= positional.span.end {
self.written_comments[i] = true;
}
}
} else {
self.format_signature(sig);
}
}
Expr::Closure(block_id) | Expr::Block(block_id) => {
self.format_block_expression(*block_id, positional.span, true);
}
Expand Down
3 changes: 3 additions & 0 deletions tests/fixtures/expected/inline_param_comment.nu
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
def fun1 [
text: string # param comment
] { $text }
3 changes: 3 additions & 0 deletions tests/fixtures/input/inline_param_comment.nu
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
def fun1 [
text: string # param comment
] { $text }
12 changes: 12 additions & 0 deletions tests/ground_truth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -736,3 +736,15 @@ fn issue76_test() {
let test_binary = get_test_binary();
run_ground_truth_test(&test_binary, "issue76");
}

#[test]
fn ground_truth_inline_param_comment_issue77() {
let test_binary = get_test_binary();
run_ground_truth_test(&test_binary, "inline_param_comment");
}

#[test]
fn idempotency_inline_param_comment_issue77() {
let test_binary = get_test_binary();
run_idempotency_test(&test_binary, "inline_param_comment");
}