first iteration of the attribute per tag POC

pointbazaar commented 4 years ago

i honestly do not know why it is creating such huge diffs. some of these methods i did not touch at all. Sometimes i changed like 3 things in a file and it marks the whole file as a diff. if someone could point out what i did wrong, that would be nice.

Anyways, here's what i have changed:

TagCreator now has html(...), body(...), head(...) methods that return a specialized class respectively.
HtmlTag extends Tag as it cannot be viewed as a ContainerTag instance, which is far too lenient and would allow for anything to be inserted with .with(...)
BodyTag and HeadTag extend ContainerTag, as they can contain almost any tag (except ofc. HtmlTag, BodyTag, HeadTag, but that is not considered in this PR.)
A bunch of tests have been changed to use the new html(...) methods (.with(...) will not work with it anymore)
As the project did not build for me (i'm on OpenJDK 14.0.1) , i also changed the Employee class

When using this code to build up Html, there would now be a clear distinction between methods that return ContainerTag and methods that return HtmlTag.

If this gets accepted, the next step would probably be to make BodyTag and HeadTag and HtmlTag unable to be used in body(...) and head(...) as that would be invalid html.

All tests are passing.

pointbazaar commented 4 years ago

i want it to have smaller diffs before marking it ready for review. But some feedback would be nice.

tipsy commented 4 years ago

i honestly do not know why it is creating such huge diffs. some of these methods i did not touch at all. Sometimes i changed like 3 things in a file and it marks the whole file as a diff. if someone could point out what i did wrong, that would be nice.

Probably line-endings?

By the way @pointbazaar, if you're interested in making major changes I'm okay with releasing a 2.0.0 version :)

pointbazaar commented 4 years ago

git config --global core.autocrlf true helped me with the too large diffs. you were right, it was the line endings

pointbazaar commented 4 years ago

PR should be ready for review now.

pointbazaar commented 4 years ago

Attributes that are legal per tag

This would be helpful for refining the types.

tipsy commented 4 years ago

Looks good @pointbazaar ! Did you see the j2html.tags.TagCreatorCodeGenerator class? You shold probably update that to remove the tags you have added explicitly now.

I see you're using some finals in your method signatures. While there's nothing wrong with that, they're not used elsewhere in the codebase (I think), so I would skip them for now.

pointbazaar commented 4 years ago

made those changes. Yes, that with the 'final' was probably just habit. There are some 'final' in the codebase, but definitely not around the places where i modified.

pointbazaar commented 4 years ago

There is a test failing however, InlineStaticResourceTest::testAllTags.

Expected: is "<body>\n    Any content\n</body>\n"
     but: was "<body>\r\n    Any content\r\n</body>\r\n"

Maybe it has to do with me being on Linux? I did not touch anything related to rendering. Maybe it has to do with the git setting somewhere up in this PR conversation.

tipsy commented 4 years ago

Maybe it has to do with me being on Linux? I did not touch anything related to rendering. Maybe it has to do with the git setting somewhere up in this PR conversation.

We should probably copy this to enable CI for all OSes: https://github.com/tipsy/javalin/blob/master/.github/workflows/main.yml

tipsy commented 4 years ago

@pointbazaar I've added a OS/JDK test matrix now, try rebasing your PR.

pointbazaar commented 4 years ago

I'm not that experienced with git and rebasing, but managed to make this branch contain only the 4 relevant commits, with (hopefully) the desired commit history. If you want, i could squash the commits also.

tipsy commented 4 years ago

If you want, i could squash the commits also.

That's okay, I can squash the commits from the GitHub UI. I wanted you to rebase to trigger the tests, and they seem to be passing on all OSes and all JDKs :)

I'll merge the PR now, thank you !

tipsy / j2html

first iteration of the attribute per tag POC #154