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

Adds support for :decorators in anon fns, defasync #1189

Merged
merged 4 commits into from
Dec 23, 2024

Conversation

ikappaki
Copy link
Contributor

Hi,

could you please review patch to support the :decorators key in anonymous fns. It addresses #1178.

The handling of :decorators has been nmoved to fn, which also ensures that the meta key is now considered when passed as metadata to the defn name. This enables support for supporting the meta key in defn derived macros such as defasync, as intended.

I've updated the defn docstring to hint that the processing is now done by fn now.

I've also added tests for fn, defn and defasync, along with a section in the python interop doc about it.

Thanks

docs/pyinterop.rst Outdated Show resolved Hide resolved
docs/pyinterop.rst Show resolved Hide resolved
(cond-> $
(meta &form) (with-meta (meta &form))))

fmeta (merge (meta fn-decl) (meta name))
Copy link
Member

Choose a reason for hiding this comment

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

Is this the correct order for how it is done in Clojure? I am indifferent towards the order, but I'd rather not have another ticket later just because it's wrong.

Copy link
Contributor Author

@ikappaki ikappaki Dec 21, 2024

Choose a reason for hiding this comment

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

This is a very interesting point I did not consider.

To clarify, I believe you are asking about the merging order: should the fn name metadata override the form (fn-decl) metadata? Specifically, should the fn name metadata key :decorators take precedence over the :decorators key in the form metadata?

From a quick survey:

  1. defn behavior: Both Clojure and Basilisp pass metadata to the fn function name.
  2. Clojure fn behavior: The fn definition uses metadata keys from the params vector only for :pre and :post conditions.

Thus based on the above, there’s no defined precedence in Clojure between fn form and name metadata. This leaves the choice up to us.

For the fn form, there are three potential places for the :decorators metadata key:

  1. The fn form itself, i.e. ^{:decorators [...]} (fn ...)).
  2. The fn name (if provided), i.e. (fn ^{:decorators [...]} name ...).
  3. The fn params, `(fn ^{:decorators [...]} [param1 ...] ...)

Since defn passes its metadata to the fn name (option 2), this seems like a strong candidate for support. Conceptually, :decorators are tied to the function, so supporting the form metadata key (option 1) also makes sense.

Conceptually to me, decorators are linked to the function, so I think support for the key in the form metadata should also be supported.

If both are supported, my view is:

  • Specific overrides general: The function name metadata key should take precedence over the form metadata key.
  • This aligns with the idea of specificity: metadata inside the fn form is more targeted than metadata outside it (but I think you can also equally argue in the opposite direction 😅)

Supporting params metadata (option 3) would also be possible, but it complicates precedence rules unnecessarily.

I prefer supporting both form and name metadata keys, with precedence given to the fn name key. Alternatively, for simplicity, we could support only the form metadata key.

What is your view?

Thanks

Copy link
Member

Choose a reason for hiding this comment

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

I prefer supporting both form and name metadata keys, with precedence given to the fn name key. Alternatively, for simplicity, we could support only the form metadata key.

I agree that we should support form and name with preference for name metadata.

tests/basilisp/test_core_macros.lpy Outdated Show resolved Hide resolved
@chrisrink10 chrisrink10 merged commit 61e86f3 into basilisp-lang:main Dec 23, 2024
12 checks passed
chrisrink10 added a commit that referenced this pull request Dec 27, 2024
…1193)

Hi,

can you please consider the follow up patch for anonymous fn decorators
support (#1189).

- Moved the note in python interop decorators doc at the top, as it
contains important policy information.
- Updated the `fn` docstring to document the form/name metadata
precedence rules established in #1189, where `name` metadata takes
precedence over `form`.
- Added new test to verify precedence rule.
-  Corrected a test that was mistakenly using `defn` instead of `fn`.


I don't think this requires a changelog entry but I can add if
necessary.

Thanks

---------

Co-authored-by: ikappaki <[email protected]>
Co-authored-by: Chris Rink <[email protected]>
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