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

Clojure-style discarding #479

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Clojure-style discarding #479

wants to merge 3 commits into from

Conversation

reo101
Copy link

@reo101 reo101 commented Apr 7, 2024

Add support for Clojure's #_ (a.k.a. discarding).

They support stacking and ignoring trailing/overstacking. See example in docs.

This could be deemed a breaking change since we no longer interpret #_ as (hashfn _).

It works by adding a new prefix to the parser, to be more specific - expands the logic for prefixes to be able to handle two-characters-wide prefixes and adds functionality for #_. It also keeps track of the number of stacked discards on each depth, applying them when dispatch-ing. The current depth depends on the calls to open-table and close-table.

I've added tests and updated the relevant docs (caused some unrelated, older changes to get included in the updated docs).


I've tried to follow CONTRIBUTING.md, please let me know if something is not right / should be done better

@technomancy
Copy link
Collaborator

Thanks for submitting this.

I have some hesitance in accepting it because I think it could make it significantly more difficult to write a correct syntax highlighter for Fennel. Before I merge it, I want to spend some time experimenting with adding support in fennel-mode, (used by me) pygments, (used by sourcehut for our official git repo hosting) pandoc, (used on fennel-lang.org) and possibly chroma (used by codeberg, another popular host for Fennel projects) to see how difficult it is.

If you want to beat me to it, that might help convince me to merge this more quickly. =)

@technomancy
Copy link
Collaborator

Oh yeah, this would also need to be supported by fnlfmt: https://git.sr.ht/~technomancy/fnlfmt

@reo101
Copy link
Author

reo101 commented Apr 11, 2024

There's also tree-sitter-fennel to touch up, I'll see what I can do there too (I have some progress there). I've been wondering though - shouldn't support for this be merged in those tools when 1.5.0 hits?

@reo101
Copy link
Author

reo101 commented Apr 11, 2024

Maybe even fennel-ls, we can set it up to send semantic tokens to indicate that a form is discarded (just like clojure-lsp does)

@technomancy
Copy link
Collaborator

shouldn't support for this be merged in those tools when 1.5.0 hits?

I don't think there's much risk of false positives firing on earlier code given that it would normally be a compilation error on earlier versions. People will want to use Emacs and fnlfmt at least on unreleased versions of Fennel. But a big part of it is not necessarily to ship the feature but to walk thru the process of implementation in order to make sure it's not adding too much work for people maintaining all the other tools to support.

Maybe even fennel-ls

Since fennel-ls uses fennel's own parser, it should get support for this automatically as long as discard-type comments get treated the way existing comments get treated, which is what we need for them to be supported in fnlfmt too.

Also wanted to say that from a technical perspective your patch is very thorough, so it's not on any technical grounds that I'm hesitating. Nice work on that.

@reo101
Copy link
Author

reo101 commented Jun 21, 2024

Hey, I've rebased on the latest main and ensured that make test succeeds again

Not really sure about the CI failure tho

@reo101
Copy link
Author

reo101 commented Jun 21, 2024

Hm, passed after regenarating the manpages 🤔

@reo101
Copy link
Author

reo101 commented Jun 21, 2024

While documenting the #_ form, I referred to it as Clojure's #_. But since it actually comes from EDN (which Clojure uses), should we call it an EDN feature instead?

@technomancy
Copy link
Collaborator

No, this feature existed in Clojure long before EDN was created.

@reo101
Copy link
Author

reo101 commented Jun 23, 2024

Some updates on my side:

  • I've rebased the PR onto main (two days ago), whilst also refactoring my logic a bit and making it cleaner (IMO), you're invited to prove me wrong 😄
  • I've managed to integrate discards into the fennel tree-sitter parser (the one used by neovim). It works great, modulo everything that is parsed using pairs (local + binding pair, let + lots of binding pairs, maps). The clojure tree-sitter parser does not enforce the pairs (it just parses everything as a sexp), but the fennel one parses the common macros and such as separate nodes with special syntax (makes common queries on those structures faster). I've also managed to make it work when we use #_ at toplevel (ignoring the subsequent toplevel form), which is something...
  • ... I still haven't achieved here (in the real parser). I am still a bit dumbfounded as to how toplevel form parsing works (the checks for the implicit module return value still confuse me a little).

I'll rebase onto main again soon and continue pondering on how to do the toplevel discarding (would not turn away any hints 😄)

Add support for Clojure's `#_`
a.k.a. [discarding](https://clojure.org/guides/weird_characters#_discard)

It supports stacking and ignoring trailing/overstacking

Example can be seen in the tests (next commit).

This could be deemed a breaking change since
we no longer interpret `#_` as `(hashfn _)`.

It works by adding a new prefix to the parser, to be more specific -
the logic for parsing prefixes is refactored to allow for early
detection of the `#_` prefix (and possibly more two-character prefixes)
and more cleanly handle the rest of prefixes.
It keeps track of the number of stacked discards on each depth, applying
them when `dispatch`-ing. The current depth depends on the calls to
`open-table` and `close-table`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants