Skip to content
This repository has been archived by the owner on Oct 27, 2022. It is now read-only.

Assign and List directive fixes and tests #20

Merged
merged 7 commits into from
Jul 17, 2014

Conversation

ppalaga
Copy link
Contributor

@ppalaga ppalaga commented Jul 16, 2014

No description provided.

@fbricon
Copy link
Member

fbricon commented Jul 16, 2014

@maxandersen the freemarker plugin has a bunch of LGPL'ed source files. Isn't it supposed to be EPL'd by now?

@ppalaga
Copy link
Contributor Author

ppalaga commented Jul 16, 2014

JavaDoc, @SInCE and license headers updated.

@fbricon
Copy link
Member

fbricon commented Jul 16, 2014

Peter, I believe the new code should be EPL'ed. Waiting for @maxandersen input about also changing all existing headers to EPL. But maybe @richardfontana can confirm too.

@ppalaga
Copy link
Contributor Author

ppalaga commented Jul 16, 2014

No problem, let's wait :)

@@ -1,6 +1,6 @@
<?xml version="1.0" encoding="UTF-8"?>
<classpath>
<classpathentry exported="true" kind="lib" path="freemarker-2.3.18.jar"/>
<classpathentry exported="true" kind="lib" path="freemarker-2.3.18.jar" sourcepath="/home/ppalaga/bin/freemarker/freemarker-2.3.18-sources.jar"/>
Copy link
Member

Choose a reason for hiding this comment

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

Local filesystem path is not portable for other developers, there should be dependency declared in pom.xml and downloaded during maven build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, {{sourcepath="/home/ppalaga/bin/freemarker/freemarker-2.3.18-sources.jar"}} needs to be removed.

With maven you mean something like this? https://github.com/ppalaga/jbosstools-base/blob/master/stacks/plugins/org.jboss.tools.stacks.core/pom.xml#L25

Copy link
Member

Choose a reason for hiding this comment

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

actually sourcepath in JDT classpathentries always use hard coded paths, even if maven downloads it automatically.
For regular Java projects, this is transparent because classpath entry attributes are stored locally within the maven classpath library. For other kind of projects were the entry shows up directly in the .classpath file, you'll always see hardcoded paths.

So removing sourcepath from .classpath is better, from a SCM perspective, but then it means developers need some extra discipline to not push .classpath when sourcepaths are modified and deal with merges when necessary.

@dgolovin
Copy link
Member

Is there a JIRA issue for this PR?

@dgolovin dgolovin closed this Jul 17, 2014
@dgolovin dgolovin reopened this Jul 17, 2014
@ppalaga
Copy link
Contributor Author

ppalaga commented Jul 17, 2014

No, there is no JIRA issue for this PR.

@fbricon
Copy link
Member

fbricon commented Jul 17, 2014

Please create one then

@maxandersen
Copy link
Member

no, freemarker and hibernate tools are still under LGPL.

@fbricon
Copy link
Member

fbricon commented Jul 17, 2014

That makes things easier then :-)

@ppalaga
Copy link
Contributor Author

ppalaga commented Jul 17, 2014

@fbricon, should I create
(a) one Jira that covers all commits or
(b) as many Jiras as there are commits in this PR or
(c) one Jira that covers just the significant commits, which is just the "Assign and List directive fixes and tests" one in this case?

I hope (c) is what you mean? :)

@fbricon
Copy link
Member

fbricon commented Jul 17, 2014

I'm fine with c)

@ppalaga
Copy link
Contributor Author

ppalaga commented Jul 17, 2014

@fbricon
Copy link
Member

fbricon commented Jul 17, 2014

Looks good to me. Java 7 isn't necessary atm, so unless something is broken, it could be applied.
Denis, have you tested it? Wanna apply it?

@dgolovin
Copy link
Member

Haven't tested it yet, just looked through the code. I'll test it and release to the master.

@dgolovin dgolovin merged commit 06a8379 into jbosstools:master Jul 17, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants