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

Polish attributes API #21

Open
Ambrevar opened this issue Jun 8, 2023 · 2 comments
Open

Polish attributes API #21

Ambrevar opened this issue Jun 8, 2023 · 2 comments

Comments

@Ambrevar
Copy link
Member

Ambrevar commented Jun 8, 2023

Follow up to #16. See #16 (comment).

The attributes API suffers from bit rot.

What needs to be tweaked:

  • attributes-default, attributes-keys-default and friends are poorly named, because they sound like they return the "default" of the attribute (which makes no sense), while it's the default attribute that's returned. Better names would be default-attribute, etc.
  • Futures start being computed in object-attributes. Can we defer to later? Can we add an option to choose when to compute it?
  • object-attributes should return attribute objects instead of the ad-hoc lists.
    We've used list because of how attributes are declared in the object-attributes methods, but really there is no need to keep lists, we can very well use lists to declare opaque attribute objects, which then would have a proper API and which upstream would not destructure manually.
  • Considering the above, we need a better way to handle attribute options. Nyxt uses position 3 and 4 in the declaration for width calculation and styling. It should be stored in the new attribute object, but where? In a plist / hash-table? We need a way to refer to these options in a non-conflicting way: let's keep in mind that other libraries may have options with the same naming but different value. So we could index by package-prefixed symbols. Example
(defmethod prompter:object-attributes ((buffer buffer) (source prompter:source))
  (declare (ignore source))
  `(("URL" ,(render-url (url buffer)) :html "..." :width 3)
    ("Title" ,(title buffer) :width 2)
    ("ID" ,(id buffer))))

would return an attribute object with slot options set to ((nyxt:html "...") (nyxt:width 3)), or something like that.

@aartaka What do you think? I remember you arguing in favour of positional options.

@Ambrevar Ambrevar mentioned this issue Jun 8, 2023
6 tasks
@aartaka
Copy link
Contributor

aartaka commented Jun 9, 2023

  • attributes-default, attributes-keys-default and friends are poorly names, because they sound like they return the "default" of the attribute (which makes no sense), while it's the default attribute that's returned. Better names would be default-attribute, etc.

Agreed!

  • Futures start being computed in object-attributes. Can we defer to later? Can we add an option to choose when to compute it?

To allow even lazier evaluation and getting attribute values only when these are required? I don't see the immediate benefit, but that sounds good.

  • object-attributes should return attribute objects instead of the ad-hoc lists.

Ad-hoc lists are so easy to work with 😞 But yeah, you're right here.

We've used list because of how attributes are declared in the object-attributes methods, but really there is no need to keep lists, we can very well use lists to declare opaque attribute objects, which then would have a proper API and which upstream would not destructure manually.

Yes!

  • Considering the above, we need a better way to handle attribute options. Nyxt uses position 3 and 4 in the declaration for width calculation and styling. It should be stored in the new attribute object, but where? In a plist / hash-table? We need a way to refer to these options in a non-conflicting way: let's keep in mind that other libraries may have options with the same naming but different value. So we could index by package-prefixed symbols. Example
(defmethod prompter:object-attributes ((buffer buffer) (source prompter:source))
  (declare (ignore source))
  `(("URL" ,(render-url (url buffer)) :html "..." :width 3)
    ("Title" ,(title buffer) :width 2)
    ("ID" ,(id buffer))))

If I got you right, you're suggesting returning a single attribute object from the prompter:object-attributes, with all the contents somehow indexed inside it. How about:

  • Returning a list of attribute objects,
  • And then having attribute to have a value slot and name slot,
  • With more slots added by the application-specific attribute subclass.

This way we have both :width ... :html ... plists (initargs for a singular attribute), simple data model (lists of predictable attributes), and the lazy computation direly necessary for this API.

@aartaka What do you think? I remember you arguing in favour of positional options.

I don't remember why I did, but right now I'm on the side of having more structure 😅

@Ambrevar
Copy link
Member Author

  • Returning a list of attribute objects,

    • And then having attribute to have a value slot and name slot,

    • With more slots added by the application-specific attribute subclass.

Oh yes! That makes total sense, I can't see why I didn't think of subclassing, I guess my nose was still stuck to the list-based API! :p

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