Skip to content

[CLEANUP] Add an interface for components of a property value #506

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

Closed
wants to merge 1 commit into from

Conversation

JakeQZ
Copy link
Contributor

@JakeQZ JakeQZ commented Feb 27, 2024

Resolves #499.

Copy link
Contributor

@oliverklee oliverklee left a comment

Choose a reason for hiding this comment

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

Looks very good to me in general.

I think this deserves an entry in the changelog.

Also, we should update the class diagram to integrate the interface and to update the associations to use the interface (maybe after #505 is merged).

* This interface has no methods to implement;
* its purpose is abstract: to allow a unique type to be specified when a `Component` is an argument of a method.
*/
interface Component {}
Copy link
Contributor

Choose a reason for hiding this comment

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

We also should make sure that the name makes sense globally (as Mermaid on GitHub doesn't allow namespaces). Maybe CssPropertyValueComponent oder PropertyValueComponent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We also should make sure that the name makes sense globally (as Mermaid on GitHub doesn't allow namespaces). Maybe CssPropertyValueComponent oder PropertyValueComponent?

I don't think whatever limitations of Mermaid there may be should dictate class naming conventions. Mermaid might in future be enhanced to show the namespaces. We should choose the most apt names for classes, interfaces and namespaces independently of whatever other tools we use.

The FQN is Value\Component. The namespace Value could perhaps be PropertyValue (or Property\Value) for clarity, though any such rename would be beyond the scope of this PR, and probably unnecessary (it would be a breaking change).

* This interface has no methods to implement;
* its purpose is abstract: to allow a unique type to be specified when a `Component` is an argument of a method.
*/
interface Component {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we should check if there are any common methods that should be part of the interface so the interface is more than just a marker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, we should check if there are any common methods that should be part of the interface so the interface is more than just a marker.

Looking again, I don't see any common methods, but do see that all of these classes extend Value, which implements Renderable. A Value can be a simple value, or a complex value comprised of simple values connected with operators.

So perhaps a better approach would be to simply use the Value type whenever any of the various subclasses is expected, and not introduce a new type (or interface) at all.

@JakeQZ
Copy link
Contributor Author

JakeQZ commented Feb 27, 2024

Looking again, I think instead we should just use the existing ancestor Value type to replace the long list of possible subtypes in the annotations, rather than adding a new empty interface.

Also, we should update the class diagram to integrate the interface and to update the associations to use the interface (maybe after #505 is merged).

Yes. (I need to get up to speed on that.)

@oliverklee
Copy link
Contributor

I think instead we should just use the existing ancestor Value type to replace the long list of possible subtypes in the annotations, rather than adding a new empty interface.

Sounds good to me.

JakeQZ added a commit that referenced this pull request Feb 28, 2024
Replace the list of various possible sub-types with the common ancestor.

Update the class diagram to indicate that `ValueList::$aComponents` no longer
has a dependency on the sub-types definitions.

Resolves #499.  Supersedes and closes #506.
JakeQZ added a commit that referenced this pull request Feb 28, 2024
Replace the list of various possible sub-types with the common ancestor.

Update the class diagram to indicate that `ValueList::$aComponents` no longer
has a dependency on the sub-types definitions.

Resolves #499.  Supersedes and closes #506.
JakeQZ added a commit that referenced this pull request Feb 28, 2024
Replace the list of various possible sub-types with the common ancestor.

Update the class diagram to indicate that `ValueList::$aComponents` no longer
has a dependency on the sub-types definitions.

Resolves #499.  Supersedes and closes #506.
@JakeQZ JakeQZ marked this pull request as draft February 28, 2024 22:33
@oliverklee oliverklee closed this in 086fa00 Mar 1, 2024
@oliverklee oliverklee deleted the cleanup/value-component-interface branch March 1, 2024 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide an interface for RuleValueList|CSSFunction|CSSString|LineName|Size|URL
2 participants