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

first iteration of the attribute per tag POC #154

Merged
merged 4 commits into from
Aug 16, 2020
Merged

first iteration of the attribute per tag POC #154

merged 4 commits into from
Aug 16, 2020

Conversation

pointbazaar
Copy link
Contributor

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 pointbazaar marked this pull request as draft August 14, 2020 06:36
@pointbazaar
Copy link
Contributor Author

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

@tipsy
Copy link
Owner

tipsy commented Aug 14, 2020

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
Copy link
Contributor Author

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

@pointbazaar
Copy link
Contributor Author

PR should be ready for review now.

@pointbazaar pointbazaar marked this pull request as ready for review August 14, 2020 08:33
@pointbazaar
Copy link
Contributor Author

Attributes that are legal per tag

This would be helpful for refining the types.

@tipsy
Copy link
Owner

tipsy commented Aug 14, 2020

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

tipsy commented Aug 15, 2020

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
Copy link
Owner

tipsy commented Aug 15, 2020

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

@pointbazaar
Copy link
Contributor Author

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
Copy link
Owner

tipsy commented Aug 16, 2020

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 tipsy merged commit f091000 into tipsy:master Aug 16, 2020
@pointbazaar pointbazaar deleted the attributes-per-tag-poc branch August 16, 2020 14:41
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