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

Generate tags from JSON file #121

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

mwanji
Copy link
Contributor

@mwanji mwanji commented Jun 28, 2018

Inspired by @mkopylec and #117 , here is a slightly different approach.

The way I see it, the generator would be in a maven plugin, so that the resulting code would be in the generated source folder and no-one would try to edit it.

The project itself would contain the configuration file (in this POC, it's JSON). This makes it easy to add tags and attributes, and also to customise the methods. For example, I used an "empty" field to output methods that don't take a value. Here's an example of the two kinds of output, where autoplay is empty and controls is not:

image

Here's what the config file looks like:

{
    "tags": [
        {
            "tag": "a",
            "attrs": [
                "href",
                "charset"
            ]
        },
        {
            "tag": "video",
            "attrs": [
                { "name": "autoplay", "empty": true },
                "controls", "height", "loop", "muted", "poster", "preload", "src", "width"
            ]
        }
    ]
}

You can see the output for yourself by running the main method in TagCreatorCodeGeneratorFromJson

This was referenced Jun 28, 2018
@tipsy
Copy link
Owner

tipsy commented Jun 28, 2018

This is a cool idea, I like it! A few thoughts:

  • These new dependencies should not be required by the final jar
  • Would make sense to create one config file per tag?
  • Would YAML be better suited than json?
{
  "tag": "video",
  "attrs": [
    {
      "name": "autoplay",
      "empty": true
    },
    "controls",
    "height",
    "loop",
    "muted",
    "poster",
    "preload",
    "src",
    "width"
  ]
}
tag: video
attrs:
- name: autoplay
  empty: true
- controls
- height
- loop
- muted
- poster
- preload
- src
- width

Edit: or TOML, if you prefer 😬

@mwanji
Copy link
Contributor Author

mwanji commented Jun 28, 2018

The generator would be in a maven plugin, so the new dependencies would have no impact on the final jar.

The config files could be split up if they turn out to be too big. But if it's mostly a list of attributes, it should be okay to keep them together.

I'm not too familiar with YAML, but I tend to find JSON more obvious, and therefore favourable, despite the verbosity.

@tipsy
Copy link
Owner

tipsy commented Jun 28, 2018

Fair enough, we can see how long it becomes before splitting it up.

I thought you meant we would call the generator using a maven plugin, but creating a plugin is okay I suppose. I have very little experience with that, would we need to publish it too?

@mwanji
Copy link
Contributor Author

mwanji commented Jun 28, 2018

Yes, the plugin would be a separate artifact. I'll have an example repo up shortly.

Would it be possible to change the use of Attr.addTo(Tag, ShortForm) to something like tag.with(ShortForm)? I currently can't figure out how to get the generics to work and changing that would make it a lot easier.

Also realising a lot of TagCreator's implementation would have to change... What would you think of generating TagCreator in the same way as the ContainerTag sub-classes, so that hand-written and generated code are no longer mixed together in one class?

@tipsy
Copy link
Owner

tipsy commented Jun 28, 2018

Would it be possible to change the use of Attr.addTo(Tag, ShortForm) to something like tag.with(ShortForm)? I currently can't figure out how to get the generics to work and changing that would make it a lot easier

Sure, this would be for version 2.0 of j2html, you don't have to worry about backward compatibility.

Also realising a lot of TagCreator's implementation would have to change... What would you think of generating TagCreator in the same way as the ContainerTag sub-classes, so that hand-written and generated code are no longer mixed together in one class?

Sounds like a good idea.

@mwanji
Copy link
Contributor Author

mwanji commented Jun 29, 2018

The plugin is at https://github.com/mwanji/j2html-maven-plugin

You can run it by installing it locally and then adding it to the build section of j2html, as I did here: https://github.com/mwanji/j2html/blob/attr-per-tag/pom.xml#L161

Sure, this would be for version 2.0 of j2html, you don't have to worry about backward compatibility.

That's a good thing, as it looks like quite a bit would have to change. For example methods such as ContainerTag#withText() can no longer return a ContainerTag, they have to return the specific type.

This can probably be done in a fairly backwards-compatible way, but it will take some thought. Also, it seems like a significant portion of the project's code would be generated, so that would be something of a shift in project philosophy or style.

@tipsy
Copy link
Owner

tipsy commented Jul 1, 2018

Sorry @mwanji, it's been a busy weekend. The current philosophy was just to keep things simple. Writing detailed rules for each tag by hand seemed like a pain to maintain, which is why j2html is pretty simple/non-strict now. If we can make this generator code work well I think it's fine to shift to a stricter paradigm.

@tipsy
Copy link
Owner

tipsy commented Aug 17, 2018

@mwanji Do you want to move forward with this?

@mwanji
Copy link
Contributor Author

mwanji commented Aug 18, 2018

Sorry, I've been quite busy the last couple months. I'm not sure I can commit to completing this any time soon.

@tipsy
Copy link
Owner

tipsy commented Aug 19, 2018

That's fine, I was just cleaning up and wondering if I should keep the PR open. Take your time.

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.

None yet

2 participants