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

configuration: Have define-configuration add just one handler per slot. #2451

Closed
wants to merge 1 commit into from

Conversation

Ambrevar
Copy link
Member

@Ambrevar Ambrevar commented Jul 12, 2022

Description

Currently calling define-configuration multiple times over the same slot has no effect.
This patch ensures there is only one handler per slot.

Discussion

  • The add-hook of nhooks should probably do this handler replacement for us.
  • Why are handler named uniquely? Shouldn't they be named after the slot, actually?

Checklist:

Everything in this checklist is required for each PR. Please do not approve a PR that does not have all of these items.

  • I have pulled from master before submitting this PR
  • My code follows the style guidelines for Common Lisp code
  • I have performed a self-review of my own code
  • My code has been reviewed by at least one peer (the peer review to approve a PR counts. The reviewer must download and test the code)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • There are no merge conflicts

EDIT: Added more discussion.

@Ambrevar
Copy link
Member Author

@aartaka OK with you?

@aadcg
Copy link
Member

aadcg commented Jul 13, 2022

Why are handler named uniquely? Shouldn't they be named after the slot, actually?

I'd say they should indeed.

If there's a single handler per slot, how could the user change the value of a slot at runtime?

I don't remember now, but don't we generate new objects when using define-configuration?

@Ambrevar
Copy link
Member Author

I don't remember now, but don't we generate new objects when using define-configuration?

True in the 2-series but no longer the case since our user-classes have hooks :) See user-classes.lisp.

@aartaka
Copy link
Contributor

aartaka commented Jul 13, 2022

No, I don't agree!

The change with %slot-value% opened for a new workflow: composing several handlers to change the value several times. I consider this a step towards a more intuitive model of configuration. If one puts several configuration forms for the same slot in their config -- they want all of those, together, modifying the value one by one!

@Ambrevar
Copy link
Member Author

If one puts several configuration forms for the same slot in their config -- they want all of those, together, modifying the value one by one!

I'm confused, what's the point of setting all the values for the same slot? I mean

(setf slot-foo 17)
(setf slot-foo 18)

is the same as

(setf slot-foo 18)

@aartaka
Copy link
Contributor

aartaka commented Jul 13, 2022

What about this?

(setf slot-foo (append 17 %slot-value%))
(setf slot-foo (append 18 %slot-value%))

This use-case is enabled by the current implementation, and I don't want to let it go!

@Ambrevar
Copy link
Member Author

Oh right...

Then I suggest one change: to have one handler per slot and to name the handler after the slot: this way it makes hook introspection and editing much easier.

@aartaka
Copy link
Contributor

aartaka commented Jul 13, 2022

"one handler per slot"?

@Ambrevar
Copy link
Member Author

Currently define-configuration defines only one handler for all the slot assignments. I suggest we add one handler for each slot assignment.

Does that make sense?

@aartaka
Copy link
Contributor

aartaka commented Jul 18, 2022

Yes, I see. Then I totally agree with this!

@Ambrevar
Copy link
Member Author

You mean you agree with my last proposal, but not this pull request as it is, right?

@aartaka
Copy link
Contributor

aartaka commented Jul 19, 2022

Yes, I don't agree with deleting the same slot handlers, but I agree with splitting slots into their own handlers!

So the course I'd be content with is removing the :place and :value from the generated handler and not deleting the handler with matching place.

@Ambrevar
Copy link
Member Author

Superseded by #2480.

@Ambrevar Ambrevar closed this Jul 27, 2022
@Ambrevar Ambrevar deleted the fix-define-configuration branch September 5, 2022 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants