Skip to content

protect literals from Air #1734

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

Open
maelle opened this issue Mar 6, 2025 · 21 comments · May be fixed by #1745 or #1816
Open

protect literals from Air #1734

maelle opened this issue Mar 6, 2025 · 21 comments · May be fixed by #1745 or #1816
Assignees
Labels
upkeep maintenance, infrastructure, and similar

Comments

@maelle
Copy link
Contributor

maelle commented Mar 6, 2025

No description provided.

@maelle maelle added the upkeep maintenance, infrastructure, and similar label Mar 6, 2025
@maelle
Copy link
Contributor Author

maelle commented Mar 6, 2025

cc @schochastics

@maelle maelle self-assigned this Mar 6, 2025
@maelle
Copy link
Contributor Author

maelle commented Mar 6, 2025

posit-dev/air#273

@maelle
Copy link
Contributor Author

maelle commented Mar 6, 2025

I need to think about this a bit more. I don't want to clutter example code with exclusion comments.

@maelle
Copy link
Contributor Author

maelle commented Mar 6, 2025

(And I don't want to have to mess up with roxygen2 to clean Rd files from exclusion comments)

@schochastics
Copy link
Contributor

Besides literals, are there other things that get messed up?

@DavisVaughan
Copy link

DavisVaughan commented Mar 6, 2025

How does this skip feature feel? posit-dev/air#279 (still subject to change). Would it help you adopt Air?

@schochastics
Copy link
Contributor

This looks amazing. I guess all we would need to do is add the toml file to the repository?

@DavisVaughan
Copy link

DavisVaughan commented Mar 6, 2025

That's the idea yea, for example I forked and added an air.toml with this to the root of rigraph:

[format]
skip = ["graph_from_literal", "from_literal"]

and then ran air format . and I see diffs like this, notice how graph_from_literal() doesn't change but other things do (ok maybe that wasn't a great example b/c that graph_from_literal() would not have changed anyways, but that's the idea)

Image

@schochastics
Copy link
Contributor

Awesome, really appreciate the work on this!

@maelle
Copy link
Contributor Author

maelle commented Mar 7, 2025

wow, this is fantastic!

One thing that might be too niche, but I'm mentioning it in case that's a general use case, is that igraph would also benefitting from excluding a regex, namely make_graph(~ (other occurrences of make_graph() don't use literals) 🤪

@maelle
Copy link
Contributor Author

maelle commented Mar 7, 2025

Maybe that makes ~ the thing to skip though 🤔

@DavisVaughan
Copy link

Hmmmm, I think for the purposes of keeping this feature fairly simple but also extremely performant, we will need to limit it to exact matches. Right now we keep the list of skip functions in a sorted list and perform a binary search to see if we get any matches, which is extremely fast. I think switching to regex would be too slow and likely not justify the amount of actual use cases there are for it. Remember we'd have to run the regex check on every single function call in your document, which can be a lot!

@maelle
Copy link
Contributor Author

maelle commented Mar 7, 2025

Thanks, this makes sense!

@maelle
Copy link
Contributor Author

maelle commented Mar 10, 2025

I tried and failed to test the extension from the branch, will wait for its merge. 😅 What I did

I switched back to using the bundled Air. 😸

@DavisVaughan
Copy link

You definitely set air.executableStrategy and air.executablePath in Positron?

@maelle
Copy link
Contributor Author

maelle commented Mar 10, 2025

Yes!

    "air.executablePath": "/home/maelle/Documents/cynkra/air/editors/code/air-vscode-0.8.0.vsix",
    "air.executableStrategy": "path",

I get the EPIPE error when I try to restart the Air server. If I try to reformat a document I get this:

Image

Maybe something is wrong with the vsix I built. 🤷‍♀ 🤔

@DavisVaughan
Copy link

You dont set air.executablePath to the vsix. You set it to the executable. i.e. if you ran cargo build you should have one in /home/maelle/Documents/cynkra/air/target/debug/air

@maelle
Copy link
Contributor Author

maelle commented Mar 10, 2025

ooooooooh 🤦 thank you!!

@maelle
Copy link
Contributor Author

maelle commented Mar 10, 2025

it works really well now, very nice feature 😸 thanks for your patience 🙏

@maelle maelle linked a pull request Mar 10, 2025 that will close this issue
@DavisVaughan
Copy link

Sorry for the delay, but Air 0.5.0 is out now (VS Code / Positron extension version 0.10.0) with this feature!

@schochastics
Copy link
Contributor

No worries and thanks a lot for adding this feature. The PR is already read: #1816 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upkeep maintenance, infrastructure, and similar
Projects
None yet
3 participants