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 @Default macro #189

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

Conversation

kevinrpb
Copy link

@kevinrpb kevinrpb commented Sep 15, 2024

Hi there! Thought I'd pitch in and contribute to this great package. This addresses #142 by creating a new macro that can be used inside @Observable classes.

The macro is implemented in a new DefaultsMacros module. The decision to do so is based on the introduction of a new dependency on swift-syntax, and to keep the main module dependency-free (specially since the syntax package adds quite a bit to build times). Please, let me know if you'd rather move it to the main module.

Settings the PR as draft for early review, but there are at least a couple things I want to do to get it ready. Also, I haven't put much thought into the naming; advice on this is appreciated.

TODO

  • Figure out if @ObservationIgnored can be added automatically as part of the macro.
    • This is not possible. There's currently no way for a macro attached to a property to add attributes to that same property.
  • Add more test cases.
  • Add docstring to the macro declaration.
  • Add info on how to use the macro to the README.

This addresses sindresorhus#142 by creating a new macro that can be used inside
`@Observable` classes.

The macro is implemented in a new `DefaultsMacros` module. The
decision to do so is based on the introduction of a new dependency on
`swift-syntax`, and to keep the main module dependency-free.
@sindresorhus
Copy link
Owner

Thanks for working on this 🙏

@sindresorhus
Copy link
Owner

The decision to do so is based on the introduction of a new dependency on swift-syntax, and to keep the main module dependency-free (specially since the syntax package adds quite a bit to build times).

👍

Tests for the macro declaration were in the wrong test target and the
test method names were not accurate.

Added another test target to actually test that the macro works when
used in an @observable class.
This is to be consistent with the `@Default` property wrapper.
In the macro tests, I had to move the @observable classes out of the
test because linter was asking to mark them as private but doing so
was causing the @observable macro to error.
@kevinrpb kevinrpb marked this pull request as ready for review September 18, 2024 17:46
@kevinrpb
Copy link
Author

I think this is now ready for review. Please, let me know about any issues/concerns.

BTW I renamed the macro to @Default so it is familiar. Let me know what you thing about this!

@kevinrpb kevinrpb changed the title Implement @ObservableDefaults macro Implement @Default macro Sep 19, 2024
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