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

Change defaults and accept per-package options. #3

Open
Ambrevar opened this issue Feb 8, 2023 · 9 comments
Open

Change defaults and accept per-package options. #3

Ambrevar opened this issue Feb 8, 2023 · 9 comments

Comments

@Ambrevar
Copy link
Member

Ambrevar commented Feb 8, 2023

I'd like to change the default accessor-name transformer to be the slot-name itself ((nclasses:make-name-transformer name)).

I'd also like to enable packages to set their defaults. Something along these lines:

(nclass:set-option :export-class-name-p t) ; Only for current package.  (And subpackages?)

@aartaka You once suggested that this set-option would be overkill and the user is better off writing their own wrapping macros.

Pros and cons?

@aartaka
Copy link

aartaka commented Feb 8, 2023

Per-package thing feels somewhat novel and unintuitive. Most use-cases are either per-application or local changes.

  • Per-application changes are OK with setfing globals or wrapping macros,
  • local changes are well with let-bound variables (kinda quirky with compile-time expansions, though) or wrapping macros.

So I'm not sure we should add this level of complexity, if there are quite conventional ways to manage options.

@Ambrevar
Copy link
Member Author

Ambrevar commented Feb 9, 2023

That's the point: we're dealing with top-level stuff here (package definitions), and we would like to not go full global.

So neither globals setting nor let bound variable would work here.

So what does it mean to be local in this case? It means applying settings only for the target package.

It's admittedly not very conventional, but in makes sense in my opinion.

@aartaka
Copy link

aartaka commented Feb 9, 2023

Yes, that is fair, things are not exactly clear in the macro-sugared-CLOS-magic lands (¬‿¬)

Okay, let it be per-package settings. But no subpackages!—there's no universal naming scheme for subpackages, and trying to guess one could be quite unintuitive. Imaginve if someone with the package naming convention like org.foo.library.core and org.foo.library.utils would try to use nclasses and rely on subpackage inferring...

@aartaka
Copy link

aartaka commented Feb 9, 2023

Hmmmmm, given that logic, I should probably remove the subpackage-related code from nsymbols (҂⌣̀_⌣́)

A new release time, I guess...

@Ambrevar
Copy link
Member Author

Ambrevar commented Feb 9, 2023

Imaginve if someone with the package naming convention like org.foo.library.core and org.foo.library.utils would try to use nclasses and rely on subpackage inferring...

Then nothing would happen. I think it's fine.

@aartaka
Copy link

aartaka commented Feb 9, 2023

But still, the logic does not do what it claims in the situations where one'd expect it to, so maybe it's not worth including it...

@Ambrevar
Copy link
Member Author

Ambrevar commented Feb 9, 2023

A docstring explaining what a subpackage is should be enough.

@aartaka
Copy link

aartaka commented Feb 9, 2023

A docstring explaining what a subpackage is should be enough.

Still uneasy about this, but okay |ω・)

@Ambrevar
Copy link
Member Author

Ambrevar commented Feb 9, 2023

I'll leave this issue open then, only changing the default accessor transformer for now.

aartaka added a commit to atlas-engineer/nyxt that referenced this issue Feb 12, 2023
In anticipation of nsymbols refactoring, inspired by
atlas-engineer/nclasses#3 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants