Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Transformation services extensions #286

Closed
wants to merge 1 commit into from
Closed

Transformation services extensions #286

wants to merge 1 commit into from

Conversation

clinique
Copy link
Contributor

@clinique clinique commented May 5, 2015

=> Ported Scale transformation service to ESH

  • enhanced scale definition capacity using Ranges
    => Factorization of Cache system from Map transformation with Scale transformation in superclass (AbstractFileTransformationService)
  • enabled cache system to operate properly on suddirectories
  • ported file localization support as exists in OH1
    => Ported JSonPath transformation service to ESH

=> Small warning supression in existing tests
=> Added JUnit tests for Scale, Map and JSonPath

@kaikreuzer
Copy link
Contributor

Hi @clinique, thanks a lot for porting this code! I assume that it has been already reviewed in openHAB 1, so I won't have to go into too many details?
As you have added new libraries, you will also need to update the about.html file (see https://www.eclipse.org/legal/epl/about.php). I will also have to request approval from the IP team for them, before I can merge.

@clinique
Copy link
Contributor Author

clinique commented May 5, 2015

Hi @kaikreuzer ,
Yes, it is currently merged in OH1 but I made a revamp of Scale transformation in order to enhance it a bit and share localization and the cache ability you introduced in Map transformation in a superclass.
Then... I wouldn't assume my code does not need a review :) made my best to have the most commented and clean as possible, but I'm not superman.
Will take action toward about.html (libraries are under Apache license).

@clinique
Copy link
Contributor Author

Squashed my commits

@kaikreuzer
Copy link
Contributor

As you have added new libraries, you will also need to update the about.html file

This is still missing, right?

@clinique
Port JSonPath transformation service to ESH
129b32f 
Port Scale transformation service to ESH
   * enhanced scale definition capacity
Factorization of Cache system from Map transformation with Scale transformation in superclass (AbstractFileTransformationService)
   * enabled cache system to operate properly on suddirectories
   * ported file definition localization as exists in OH1
Small warning supression in existing tests
Added tests for Scale, Map and JSonPath

Signed-off-by: Gaël L'hopital <glhopital@gmail.com>

Add OSGI description files for Scale and JSonPath transformation services

Signed-off-by: Gaël L'hopital <glhopital@gmail.com>

Improved Scaletransform to move scale file interpretation on load instead of transform

Signed-off-by: Gaël L'hopital <glhopital@gmail.com>

Personal code review

Signed-off-by: Gaël L'hopital <glhopital@gmail.com>

Modify about.html to reflect licenses of uderpinned libraries.
Moved to last version of json-path and json-smart

Signed-off-by: Gaël L'hopital <glhopital@gmail.com>
@clinique
Copy link
Contributor Author

Done

@kaikreuzer kaikreuzer mentioned this pull request Jun 11, 2015
Merged
@kaikreuzer
Copy link
Contributor

Sorry @clinique for not yet further processing this. As mentioned in #340, I would actually suggest to refactor this into separate bundles, so that users do not have to include all transform dependencies, but only the ones that they want to use. Let's first wait until #340 is merged and then it would be great if you could refactor this contribution accordingly - sorry for the inconvenience, but it is all for keeping a clean architecture on the long run :-)

@clinique
Copy link
Contributor Author

@kaikreuzer , no problem, there is no hurry and keeping the project clean is key

@kaikreuzer kaikreuzer mentioned this pull request Jun 16, 2015
Closed
sja
sja reviewed Jun 17, 2015
View reviewed changes
...st/java/org/eclipse/smarthome/core/transform/internal/JSonPathTransformationServiceTest.java
/**
* @author Gaël L'hopital
*/
public class JSonPathTransformationServiceTest extends AbstractTransformationServiceTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to see here a failure test which produces a TransformationException as well.
Maybe you can copy it from my PR: https://github.com/sja/smarthome/commit/003f7218739ec1912bf4151dfe15f8c937a8d12a

@sja
Copy link
Contributor

sja commented Jun 17, 2015

Ok, I'll fork it as you suggested. As far as I saw, the changes of #340 have to be adopted as well, which I can (have to) do as well. :-)

@clinique
Copy link
Contributor Author

Yes, I guess it will lengthen a bit the merging process, but from the ground it's a good point.

@kaikreuzer
Copy link
Contributor

@sja Yes, it could make sense to base your PR on #340 - I am only waiting for approval by the IP team before I can merge this.

@kaikreuzer
Copy link
Contributor

Fyi: #340 has just been merged!

@clinique
Copy link
Contributor Author

Ok, I will close this PR and make two distinct integrating #340 : one for Localizable file along with Scale transformation service and another with JSonPath transformation service

@clinique clinique closed this Jun 19, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@clinique @kaikreuzer @sja