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

Implement multiline delimiter catpures #837

Closed
wants to merge 3 commits into from
Closed
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
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ futures = "0.3.28"
itertools = "0.11"
js-sys = "0.3"
libloading = "0.8.4"
log = "0.4"
log = { version = "0.4", features = ["kv"] }
nickel-lang-core = { version = "0.8.0", default-features = false }
predicates = "3.0"
pretty_assertions = "1.3"
Expand Down
7 changes: 7 additions & 0 deletions topiary-cli/tests/samples/expected/rust.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,3 +109,10 @@ impl MyTrait for MyStruct {
// ... logic ...
}
}

fn main() {
self.append(Atom::Empty,
node,
predicates,
);
}
7 changes: 7 additions & 0 deletions topiary-cli/tests/samples/input/rust.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,3 +110,10 @@ impl MyTrait for MyStruct {
// ... logic ...
}
}

fn main() {
self.append(Atom::Empty,
node,
predicates
);
}
86 changes: 59 additions & 27 deletions topiary-core/src/atom_collection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,15 @@ impl AtomCollection {
node,
predicates,
),
"append_multiline_delimiter" => {
if self.in_multiline_context(node) {
self.append(
Atom::Literal(requires_delimiter()?.to_string()),
node,
predicates,
)
}
}
"append_empty_softline" => {
self.append(Atom::Softline { spaced: false }, node, predicates);
}
Expand All @@ -251,6 +260,15 @@ impl AtomCollection {
node,
predicates,
),
"prepend_multiline_delimiter" => {
if self.in_multiline_context(node) {
self.prepend(
Atom::Literal(requires_delimiter()?.to_string()),
node,
predicates,
)
}
}
"prepend_empty_softline" => {
self.prepend(Atom::Softline { spaced: false }, node, predicates);
}
Expand Down Expand Up @@ -591,6 +609,22 @@ impl AtomCollection {
self.append.entry(target_node.id()).or_default().push(atom);
}

/// Indicates whether we are in a multiline context or not.
/// # Arguments
///
/// * `node` - The node to which the atom applies.
/// # Returns
///
/// A boolean indicating whether a given node has a a parent labelled as multi-line.
/// If the provided node has no parent, the function returns `false`.
fn in_multiline_context(&self, node: &Node) -> bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there seems to be a lot of variance between referring to multilines, would help to normalize for consistency:

  • multiline
  • multi-line
  • multi_line

let parent_id = node.parent().map(|p| p.id());

parent_id
.map(|pid| self.multi_line_nodes.contains(&pid))
.unwrap_or(false)
}

/// Expands a softline atom to a hardline, space or empty atom depending on
/// if we are in a multiline context or not.
///
Expand All @@ -612,34 +646,32 @@ impl AtomCollection {
///
/// A new atom after expanding the softline if applicable.
fn expand_multiline(&self, atom: Atom, node: &Node) -> Atom {
if let Atom::Softline { spaced } = atom {
if let Some(parent) = node.parent() {
let parent_id = parent.id();

if self.multi_line_nodes.contains(&parent_id) {
log::debug!(
"Expanding softline to hardline in node {} with parent {}: {}",
node.display_one_based(),
parent_id,
parent.display_one_based()
);
Atom::Hardline
} else if spaced {
log::debug!(
"Expanding softline to space in node {} with parent {}: {}",
node.display_one_based(),
parent_id,
parent.display_one_based()
);
Atom::Space
} else {
Atom::Empty
}
} else {
Atom::Empty
}
let Atom::Softline { spaced } = atom else {
return atom;
};
let Some(parent) = node.parent() else {
return Atom::Empty;
};
let parent_id = parent.id();

if self.multi_line_nodes.contains(&parent_id) {
log::debug!(
parent_id;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this does tracing style key/value logging hence the activation of the kv feature flag in log, happy to remove. Made debug logs a bit cleaner for me.

"Expanding softline to hardline in node {}: {}",
node.display_one_based(),
parent.display_one_based()
);
Atom::Hardline
} else if spaced {
log::debug!(
parent_id;
"Expanding softline to space in node {}: {}",
node.display_one_based(),
parent.display_one_based()
);
Atom::Space
} else {
atom
Atom::Empty
}
}

Expand Down
11 changes: 11 additions & 0 deletions topiary-queries/queries/rust.scm
Original file line number Diff line number Diff line change
Expand Up @@ -205,3 +205,14 @@
(
"," @prepend_antispace
)

; append trailing comman to newline delimited arguments
(arguments
(#delimiter! ",")
(_) @append_multiline_delimiter
.
","? @do_nothing
.
")"
Comment on lines +214 to +216
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's an idempotence check failure here that's likely clashing with the @append_spaced_softline after the comma:

; Append softlines, unless followed by comments.
(
[
","
";"
] @append_spaced_softline
.
[(block_comment) (line_comment)]* @do_nothing
)

Logically, the precedence should be to handle the append delimiter before handling the sofline.

.
)
Loading