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

[jinja] Jinja Transformation #4943

Merged
merged 1 commit into from
Feb 23, 2019
Merged

[jinja] Jinja Transformation #4943

merged 1 commit into from
Feb 23, 2019

Conversation

jochen314
Copy link
Contributor

This is a new transform using jinja.

This transform is needed to fully support mqtt homeassistant discovery.

See also #4927

@jochen314 jochen314 requested a review from a team as a code owner February 19, 2019 22:06
@wborn wborn changed the title [jinja] JINJA transform initial contribution [jinja] Jinja Transform initial contribution Feb 20, 2019
@davidgraeff
Copy link
Member

Could you please add your "Signed-off-by" :)

@@ -0,0 +1,2 @@
eclipse.preferences.version=1
Copy link
Member

Choose a reason for hiding this comment

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

Please don't commit eclipse settings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am used to more .gitirnore entries....

lib/jackson-core-2.9.8.jar,
lib/jackson-databind-2.9.8.jar,
lib/jackson-annotations-2.9.8.jar,
lib/commons-lang3-3.8.1.jar
Copy link
Member

Choose a reason for hiding this comment

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

The dependency list is just quite huge, but there is nothing we can do about that, I guess? Maybe you are only using a subset of "jinjava" and we can reduce this list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked again and all are needed to run the simple test examples :-(

* Home assistant [templating](https://www.home-assistant.io/docs/configuration/templating/).


## Copyright
Copy link
Member

Choose a reason for hiding this comment

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

Remove that part from the readme, please. It is sufficient to have NOTICE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

* <p>
* The implementation of {@link TransformationService} which transforms the input by Jinja2 Expressions.
*
* @author Jochen Klein
Copy link
Member

Choose a reason for hiding this comment

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

For all author tags: We have the pattern: "author name - Initial contribution"

The static checker will also error out on this detail

@@ -0,0 +1 @@
Bundle resources go in here!
Copy link
Member

Choose a reason for hiding this comment

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

You don't need that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was in copy template (javascript tranformer) :-)


## Further Reading

* Wikipedia on [Jinja](https://en.wikipedia.org/wiki/Jinja_(template_engine).
Copy link
Member

Choose a reason for hiding this comment

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

There is a ")" missing at the end

Signed-off-by: Jochen Klein <git@jochen.susca.de>
@davidgraeff
Copy link
Member

Thanks :)

@@ -0,0 +1,107 @@
/**
* Copyright (c) 2014,2018 Contributors to the Eclipse Foundation
Copy link
Contributor

Choose a reason for hiding this comment

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

The Jenkins build is failing because of wrong license headers. Can you please run mvn license:format in the root directory of this project and come up with a new PR for your fixed headers? Thanks.

You mayb want to take care about the warning too.

[ERROR] Code Analysis Tool has found: 
 4 error(s)! 
 2 warning(s) 
 0 info(s)
[ERROR] org.openhab.transform.jinja.internal.profiles.JinjaTransformationProfileFactory.java:[2]
Header line doesn't match pattern ^ \* Copyright \(c\) 2010-2019 Contributors to the openHAB project$
[ERROR] org.openhab.transform.jinja.internal.profiles.JinjaTransformationProfile.java:[2]
Header line doesn't match pattern ^ \* Copyright \(c\) 2010-2019 Contributors to the openHAB project$
[ERROR] org.openhab.transform.jinja.internal.JinjaTransformationService.java:[2]
Header line doesn't match pattern ^ \* Copyright \(c\) 2010-2019 Contributors to the openHAB project$
[WARNING] .transform.jinja/ESH-INF/config/jinjaProfile.xml:[3]
There were whitespace characters used for indentation. Please use tab characters instead
[ERROR] .transform.jinja/ESH-INF/config/jinjaProfile.xml:[0]
The file ESH-INF/config/jinjaProfile.xml isnt included in the build.properties file. Good approach is to include all files by adding `ESH-INF/` value to the bin.includes property.

Copy link
Member

Choose a reason for hiding this comment

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

Oh my word, I haven't caught that as well (probably because I'm also compiling without tests and checks at the moment). Should we revert this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we revert this PR?

No, we should come up with a follow-up PR to fix it.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it wasn't an issue if it the addon was reviewed by 2 maintainers:

New add-on contributions should be reviewed by 2 maintainers - always feel free to ping @kaikreuzer to do a second/final approval.

There's now https://github.com/openhab/openhab2-addons/pull/4984

Copy link
Member

Choose a reason for hiding this comment

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

True. I'm just eager to get the required parts in for full MQTT HomeAssistant support and I know that you guys are busy with the build system and IDE setup.

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/next-generation-design-a-paper-ui-replacement-proposal/64930/206

@wborn wborn added this to the 2.5 milestone Feb 28, 2019
Pshatsillo pushed a commit to Pshatsillo/openhab-addons that referenced this pull request Jun 19, 2019
Signed-off-by: Jochen Klein <git@jochen.susca.de>
Signed-off-by: Pshatsillo <pshatsillo@gmail.com>
ne0h pushed a commit to ne0h/openhab-addons that referenced this pull request Sep 15, 2019
Signed-off-by: Jochen Klein <git@jochen.susca.de>
Signed-off-by: Maximilian Hess <mail@ne0h.de>
@kaikreuzer kaikreuzer changed the title [jinja] Jinja Transform initial contribution Jinja Transformation Dec 8, 2019
@kaikreuzer kaikreuzer changed the title Jinja Transformation [jinja] Jinja Transformation Dec 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants