Skip to content

Implementation of Attributes-per-Tag via Interfaces #156

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

Merged
merged 19 commits into from
Aug 31, 2020
Merged

Implementation of Attributes-per-Tag via Interfaces #156

merged 19 commits into from
Aug 31, 2020

Conversation

pointbazaar
Copy link
Contributor

…efault methods. Drawback: 'get()' method that does nothing, typecasting and many classes and interfaces

…efault methods. Drawback: 'get()' method that does nothing, typecasting and many classes and interfaces
@pointbazaar pointbazaar marked this pull request as draft August 16, 2020 16:34
@pointbazaar
Copy link
Contributor Author

pointbazaar commented Aug 16, 2020

Can i get some feedback on this implementation?
If you like it, i could try to work down the list of attributes Attributes
in this draft PR until all the attributes are implemented.

What bothers me the most is not being able to get a type of the actual class instance when implementing default methods in an
interface. Maybe there is another way?
The way I'm doing it now to is to use the type parameter of the interface to cast 'this' to 'T',
so the default method in the interface can return the correct type.

Currently the type of the tags is also watered down to the type they are extending when using any attribute methods from the base class. This could be avoided by maybe generifying the ContainerTag. But i wanted to get feedback on this direction first, as there would be quite the number of new classes and interfaces.

@tipsy
Copy link
Owner

tipsy commented Aug 21, 2020

Will have a look tomorrow !

@tipsy
Copy link
Owner

tipsy commented Aug 22, 2020

Looks fine @pointbazaar. My only concern is it will be a lot to maintain, but if you can manage it then it's fine by me :)

@pointbazaar
Copy link
Contributor Author

Well atleast the classes (without the 'implements XYZ' ) themselves could be generated.
Then adding the 'implements XYZ' for each attribute would be some work, but it could be done.

I think this PR conflicts with PR121 regarding how
attributes-per-tag should be implemented. In the other PR it seems to me that each class would have a method per
custom attribute, while in this PR, it would be implemented via interfaces.

I'm not well versed with how this would be compiled, but i would assume that generating a method per custom attribute
would result in significantly more bytecode. A lot of the attributes apply to 5 or more html tags.
Also i am unsure which of the 2 would result in the best performance.

In this PR, only the classes themselves would be generated once, then the interfaces would be added and then further implementation which would maybe require tag-specific implementations would be done by hand.
As shown with html(...) implementation in TagCreator.

I wonder if there would be enough opportunities to make generating once and then handcoding the rest worthwhile.

So far, i only stumbled upon [html, head, body] as candidates which could benefit from custom implementations, as they
are special in how they can be composed.

@pointbazaar
Copy link
Contributor Author

pointbazaar commented Aug 22, 2020

An important consideration would also be that if you have n tags and m attributes per tag, and x methods added to a tag-class for having an attribute,
it would take about 3 * n * m * z lines with the other PR, but only n * m lines with this PR. because the interface would contain the methods only once for each attribute. This would be important for further extensibility. Because after some time it would be impractical to generate all those methods. But this would assume a large amount of methods per attribute, like > 10 or something. paired with a tag that has 10 custom attributes it can have, then there would be 100 methods in that class, which would be rather large. And depending on the bytecode generated, would bloat the j2html jar unneccessarily.

@pointbazaar
Copy link
Contributor Author

maybe the 2 approaches can be mixed, generating both interfaces and tag specific classes that 'implement' those interfaces.

@tipsy
Copy link
Owner

tipsy commented Aug 23, 2020

#121 has been dead for a very long time, so I wouldn't worry about that. You can go ahead without code generation, then switch to it later if it makes sense ?

…rent tags, and change TagCreatorCodeGenerator to reflect those changes.
@pointbazaar
Copy link
Contributor Author

Now the classes for the different tags can be generated in their most basic form.

… This is a step to make it more typesafe. the constructing methods now return a custom type for each tag being constructed.
@pointbazaar
Copy link
Contributor Author

Now the TagCreatorCodeGenerator will generate methods that return the custom type for each tag.

@pointbazaar
Copy link
Contributor Author

pointbazaar commented Aug 23, 2020

  • EmptyTag generified
  • ContainerTag generified
  • generate interfaces differently depending on if the attribute has an argument or not:
    e.g.
withX(final String argument){...}

or

withX(final boolean enabled){...}

What remains to do:

  • double-check the attributes and which tags they apply to
  • generate the interfaces in a way that is consistent with the rest of the project (like isHidden(), withCondHidden(...) ), because currently it is just one method e.g. withDownload(final boolean ...) which is more to write
  • make a single main() which would generate all required classes and interfaces , so that the files generated are consistent with the generator for each generator run. (canceled)
  • prune some attributes which are not suitable for all tags of their kind from Tag.java / EmptyTag.java / ContainerTag.java if there are any

… the rest of the project. These methods should now be generated.
@pointbazaar
Copy link
Contributor Author

pointbazaar commented Aug 24, 2020

  • added all global attributes in Tag.java
  • changed the Code Generator for the Interfaces to generate methods consistent with the rest of the project.
  • pruned attribute methods from Tag.java that were not valid on all HTML tags

What remains:

  • Using correct default attribute values for attributes without argument e.g. .isAutoFocus() should generate <$TAG autofocus>...
    but .isAutocomplete() should generate <$TAG autocomplete="on"> ... (This can be adjusted in AttributeList.java vie AttrD constructor. )
  • Writing Tests for Attributes-Per-Tag, testing that the generated methods indirectly generate the correct HTML
  • Change ConvenienceMethodTest.java to contain only the tests for global HTML attributes (those defined within Tag.java) and let the rest be tested by other Methods. (canceled, as this PR is getting too big)
  • handle "accept-charset" attribute (currently disabled due to the '-') (canceled, as this PR is getting too big)

@pointbazaar
Copy link
Contributor Author

pointbazaar commented Aug 24, 2020

  • Wrote Tests for Attributes-Per-Tag, covering some of the generated methods
  • Went through the Attributes list from W3Schools and enter the attribute property if they have an argument or not

Probably some special cases have been missed and also some attributes have not even been included here, most of them because of a dash ('-') in their name, which would force me to make some decisions as to how their interfaces and attribute-specific methods should be named.

I'm facing some tradeoffs with this PR currently:

  • writing many tests for attributes-per-tag would mean wasted effort if there is a decision
    to change the method names later and would make it complicated (hand-refactoring and then re-generating the interfaces would be required if one would not want to rewrite those tests)
  • it would be very convenient to have special with$ATTR(int $ATTR) Methods for example for withTabindex where one could enter a number instead of a string. Because the attribute is a number already. This would spare people generating their HTML the x+"", all too common in some codebases. However this would complicate code generation even further, and it already looks like spaghetti code and could use some refactoring. However for withTabindex this has already been implemented, as it is handcoded in Tag.java.

However i am confident that this PR in it's current state represents an improvement in type-safety and convenience for users of J2HTML and would like to have it reviewed. It has been going on for too long and has collected too many commits already.
The gaps left by this PR can easily be added in later on and in my opinion feel like an omission and not a regression for the API.

@pointbazaar pointbazaar marked this pull request as ready for review August 24, 2020 21:12
@pointbazaar pointbazaar changed the title concept for implementation of attributes per tag via interfaces and d… Implementation of Attributes-per-Tag via Interfaces Aug 24, 2020
@tipsy
Copy link
Owner

tipsy commented Aug 24, 2020

It's gotten quite big, yeah. I will go through it in the weekend.

@tipsy
Copy link
Owner

tipsy commented Aug 30, 2020

I didn't fully read through the generator code, but the generated code looks good.

It would be helpful to add a section in the readme, or create a CONTRIBUTING.md file to describe the new architecture and the role of the generators, as everything is a bit more complex now. This can be done in a separate PR though.

@pointbazaar
Copy link
Contributor Author

PR is out!

@pointbazaar pointbazaar requested a review from tipsy August 31, 2020 13:57
@tipsy tipsy merged commit 312cabe into tipsy:master Aug 31, 2020
@tipsy
Copy link
Owner

tipsy commented Aug 31, 2020

Great work :)

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