Skip to content
This repository has been archived by the owner on Apr 11, 2018. It is now read-only.

RF-12802: page fragments repository moved here #55

Merged
merged 4 commits into from Oct 29, 2013
Merged

RF-12802: page fragments repository moved here #55

merged 4 commits into from Oct 29, 2013

Conversation

jhuska
Copy link
Contributor

@jhuska jhuska commented Oct 17, 2013

No description provided.

<name>RichFaces Build Version Management</name>

<description>
<!-- JBoss, Home of Professional Open Source Copyright 2013, Red Hat, Inc.
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather we not reformat the license header.

@bleathem
Copy link
Member

this looks really good. IMO we should clean up the pom.xml formatting and then merge it. We are still in the Alpha stage with RichFaces 5, so we will have time to re-factor the fragments once we start playing.

One question about packaging: How will users consume these fragments? Are we packaging them in a separate jar? In the richfaces.jar?

@lfryc
Copy link
Contributor

lfryc commented Oct 23, 2013

+1 separated JAR

I can foresee two solutions:

  1. separated maven module, like richfaces5/build/page-fragments producing artifact richfaces-page-fragments (richfaces-page-fragments.jar)

  2. source subfolder - like richfaces5/framework/src/main/page-fragments producing artifact richfaces:page-fragments (richfaces-page-fragments.jar)

(2) is actually solution for cyclic dependencies, which is I believe not our case at the moment, @jhuska ?

@jhuska
Copy link
Contributor Author

jhuska commented Oct 29, 2013

I have updated the pull request according to your comments.

  • formatted the build/pom.xml according to the JBoss rules
  • moved page-fragments under build/page-fragments
  • removed cyclic dependency of page-fragments to richfaces itself
  • rebased against master

@lfryc
Copy link
Contributor

lfryc commented Oct 29, 2013

I have reviewed the dependencies of the page-fragments and found out you suggest to add these dependencies:

joda-time:2.3
- DateTime

commons-lang:2.6
- Validate.isTrue
- StringEscapeUtils.unescapeJava

json-simple:1.1.1
- JSONParser

org.jodah:typetools:0.3.0
- TypeResolver.resolveRawArguments

I'm fine with DateTime, since it's going to be standardized - even though we may consider making it optional dependency and provide alternative in form of java.util.Date.

I didn't found reason to leave Validate.isTrue - this should be replaced by more verbose but dependency free code.

I did some googling for alternatives to StringEscapeUtils.unescapeJava and found out few suggestions for dependency free Java-unescaping.

JSONParser seems to be necessary (even though we don;t use it extensively yet), but I would favor standard APIs in form of JSR-353.

I believe TypeResolver can be re-written from scratch, but since it's one-class ASL dependency, we might consider shading it.


However I believe we are ready to merge page fragments with dependencies as they are and create an issue to address described shortcomings ^. Wdyt @bleathem ?

I have created this issue to address it: https://issues.jboss.org/browse/RF-13300

@lfryc
Copy link
Contributor

lfryc commented Oct 29, 2013

I also believe you forgot to specify version for commons-lang.

Page fragment sources are also missing license headers:
https://issues.jboss.org/browse/RF-13299

Once merged, we should make sure we favor Page Fragments in Framework Tests (we may start to refactor newly introduced components so that we won't dive into any refactoring): https://issues.jboss.org/browse/RF-13301

@lfryc lfryc merged commit 93a3891 into richfaces:master Oct 29, 2013
@lfryc
Copy link
Contributor

lfryc commented Oct 29, 2013

@lfryc
Copy link
Contributor

lfryc commented Oct 29, 2013

Thanks Juraj for all your hard work on Page Fragments, this is huge achievement!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants