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

Added named source blocks #831

Merged
merged 1 commit into from
Apr 26, 2024
Merged

Added named source blocks #831

merged 1 commit into from
Apr 26, 2024

Conversation

jokesper
Copy link
Contributor

@jokesper jokesper commented Apr 3, 2024

This PR adds named defsrc blocks.
To keep backwards compatibility I decided to name them defsrc'.

There are some design decisions which could be changed:

  1. defsrc' instead of something else like defsrc-named.
    But I found finding the right name is hard. :|
  2. defsrc' instead of expanding defsrc.
    Expanding defsrc through sub expressions
    would be possible since no key could be a sub expression.
    Furthermore it would allow future expansion (e.g.: fallthrough per source)
(defsrc (name qwerty)
  q w e r t y
)

I opted for defsrc' as it was the easiest. Also note that keys not defined in this source are passthrough to other layers (_). This was simply the easiest way to implement.
Let me know if you find some issues.

Copy link
Member

@slotThe slotThe left a comment

Choose a reason for hiding this comment

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

This looks great, thanks!

Expanding defsrc through sub expressions would be possible since no key could be a sub expression. Furthermore it would allow future expansion (e.g.: fallthrough per source)

I think I would favour this solution.

Syntax-wise, perhaps something this would fit:

(defsrc {:name my-name}
  …)

(this is akin to how Clojure functions can be given an attribute set with some metadata information)

@jokesper
Copy link
Contributor Author

Wouldn't something like

(defsrc (name my-name)
 ...)

make more sense.
I.e. let defsrc take not just keycodes but also defsrc options. This wouldn't introduce new syntax.
The only difference would be that it might be unclear what you mean, when applied to other functions

@slotThe
Copy link
Member

slotThe commented Apr 20, 2024

I figured that {:name my-name} might better communicate the intent of these being additional options, though I don't think there's anything wrong with (name my-name) either. No strong opinions on my side; pick whichever form you like best.

@jokesper
Copy link
Contributor Author

jokesper commented Apr 20, 2024

Sry my mobile UI was messed up.
Wouldn't the braces cause problems, since they aren't keywords and would reference buttons?

@slotThe
Copy link
Member

slotThe commented Apr 21, 2024

Wouldn't the braces cause problems, since they aren't keywords and would reference buttons?

This parser would run before the button parser, so this should not cause any issues. Instead of the current

  , try (symbol "defsrc") *> (KDefSrc <$> defsrcP)

we would have something like

  , try (symbol "defsrc") *> (KDefSrc <$> defsrcAttrSetP <*> defsrcP)

which would not cause any ambiguities

@jokesper
Copy link
Contributor Author

True, but It would be best for the attribute set to be optional to allow for backwards compatibility.
But something like:

(defsrc { } )

Which would previously define the source keys to be the braces, would now be an empty source with an attribute set.
with parenthesis this doesn't happen, since they already have to be escaped.

@jokesper
Copy link
Contributor Author

jokesper commented Apr 21, 2024

I was just looking at the code and noticed how tap-macro handles it.
Maybe something like

(defsrc :name my-name
    a b c)

would be ideal.
So simply leave out the braces and signal the optional argument via the colon from keywordP
Though I did not restrict it to always be on the end (like tap-macro), since I already had that code and it was a one line change from my uncommited code

@slotThe
Copy link
Member

slotThe commented Apr 21, 2024

True, but It would be best for the attribute set to be optional to allow for backwards compatibility. But something like:

(defsrc { } )

Which would previously define the source keys to be the braces, would now be an empty source with an attribute set. with parenthesis this doesn't happen, since they already have to be escaped.

Seems more logical to me to simply not parse empty attribute blocks. Either way, I also think that

(defsrc :name my-name
    a b c)

looks fine.

@jokesper
Copy link
Contributor Author

I implemented the changes. Please take another look.

src/KMonad/Args/Parser.hs Outdated Show resolved Hide resolved
@jokesper
Copy link
Contributor Author

I applied your suggested changes. Please take a look again.

@slotThe slotThe merged commit 1a229b1 into kmonad:master Apr 26, 2024
13 checks passed
@slotThe
Copy link
Member

slotThe commented Apr 26, 2024

Thank you!

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.

None yet

2 participants