Skip to content

Replaced Vector with ArrayList, fixed Generics in Utils #6

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.

Sign up for GitHub

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

Closed
miho wants to merge 5 commits into from
Closed

Replaced Vector with ArrayList, fixed Generics in Utils #6

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c32a19e
Fixed collection usage:
miho Feb 11, 2018
8fc71cc
fixed gradle
miho Feb 11, 2018
e9e340f
fixed gradle
miho Feb 11, 2018
d2306e7
fixed package name (uses new dbresearch prefix now)
miho Feb 12, 2018
6e34166
temporarily removing gradle wrapper (will stay in our fork)
miho Feb 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,4 @@ hs_err_pid*
# Gradle stuff
build/
.gradle/
/.nb-gradle/
Copy link
Copy Markdown
Collaborator

@mateuszpawlik mateuszpawlik Feb 12, 2018

Choose a reason for hiding this comment

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

The file has not changed. This should not be part of the commit.

Copy link
Copy Markdown
Author

@miho miho Feb 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

actually it has changed. it prevents ide users from accidentally committing local ide related changes.

Copy link
Copy Markdown
Collaborator

@mateuszpawlik mateuszpawlik Feb 12, 2018

Choose a reason for hiding this comment

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

Sorry for that. My mistake. Thanks for the explanation.

14 changes: 11 additions & 3 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,28 @@
apply plugin: 'java'

sourceCompatibility = '1.8'
[compileJava, compileTestJava]*.options*.encoding = 'UTF-8'

task wrapper(type: Wrapper) {
gradleVersion = '4.5.1'
distributionUrl = "https://services.gradle.org/distributions/gradle-$gradleVersion-all.zip"
}

// Dependency repository.
repositories {
mavenCentral()
}

// Project dependencies. Currently only for unit tests.
dependencies {
testCompile 'junit:junit:4.12',
'com.google.code.gson:gson:2.8.0'
testCompile 'junit:junit:4.12';
testCompile 'com.google.code.gson:gson:2.8.0'
Copy link
Copy Markdown
Collaborator

@mateuszpawlik mateuszpawlik Feb 12, 2018

Choose a reason for hiding this comment

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

What's the difference here? If only syntax, then it should go away.

}

// Specify main class for the JAR file.
jar {
manifest {
attributes 'Main-Class': 'util.CommandLine'
attributes 'Main-Class': 'at.unisalzburg.apted.util.CommandLine'
Copy link
Copy Markdown
Collaborator

@mateuszpawlik mateuszpawlik Feb 12, 2018

Choose a reason for hiding this comment

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

I post it here, because it's the first occurrence of that change.

Since it's the second pull request about packaging, I've decided to go for it. There are however two issues.
(1) Will such change affect the current users of the library? If yes, what are the options to deal with it?
(2) I'd prefer 'at.unisalzburg.dbresearch' instead of 'at.unisalzburg' prefix.

Copy link
Copy Markdown
Author

@miho miho Feb 12, 2018

Choose a reason for hiding this comment

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

Sure it will affect current users. Changing the package name is a breaking API change. On the other hand, the current situation is causing similar problems. To simplify the migration you could ship a version that contains both, the old packages and the new ones. In the old, you add a warning that will tell users that they have to migrate to the new package name.

}
}

Expand Down
5 changes: 5 additions & 0 deletions gradle/wrapper/gradle-wrapper.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
distributionBase=GRADLE_USER_HOME
Copy link
Copy Markdown
Collaborator

@mateuszpawlik mateuszpawlik Feb 12, 2018

Choose a reason for hiding this comment

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

This file is automatically generated and should go away from the repository.

Copy link
Copy Markdown
Author

@miho miho Feb 12, 2018

Choose a reason for hiding this comment

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

This file belongs to the gradle wrapper. The fact that it is automatically created isn't relevant here. The sole purpose is to reduce the complexity of the build process and to prevent potential problems with incompatible gradle versions.

Repositories that contain gradle wrapper scripts and configs do not require a manual gradle download. Users can simply run ./gradlew.sh assemble to compile the code. If the required gradle version is not already installed it will be downloaded.

This is a well accepted procedure. You can read the documentation here: https://docs.gradle.org/current/userguide/gradle_wrapper.html

Copy link
Copy Markdown
Collaborator

@mateuszpawlik mateuszpawlik Feb 13, 2018

Choose a reason for hiding this comment

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

Thank you for your expertise. I've read the documentation you linked to. I'm not Java developer but I don't agree with you. Automatically generated files don't have place in my repository. I've looked around and found this constructive discussion: https://stackoverflow.com/questions/20348451/why-should-the-gradle-wrapper-be-committed-to-vcs

Please read through it and propose a solution that suits you. I personally like the answer by Tomas Bjerre. I'm sure we can find a solution.

Copy link
Copy Markdown
Author

@miho miho Feb 13, 2018

Choose a reason for hiding this comment

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

From a theoretical point of view I understand your aversion against automatically generated files. But I think you are giving the automatic vs. non-automatic discussion way too much attention. In my opinion it should be a pragmatic discussion about pros vs. cons. The discussion you referenced is more about opinions than pragmatic solutions.

In particular, the comparison of the JDK (a platform dependent dependency that weights roughly 1GB if you want your library to work on Linux, Mac and Windows, only counting x64 arch) and a platform independent dependency that is roughly 60-70KB is very strange and misleading. As a pragmatic developer I always look at pros vs. cons. The cons of a nearly 1GB binary blob outweighs by far the benefit it could potentially have. A 70KB platform independent way to solve my dependency problems has certainly more pros than cons.

In his reply, Thomas asks

"Do you also commit all your dependencie libraries?"

This is where he completely misses the intention behind the Gradle wrapper. It is not some random dependency. It bootstraps your build process. All of your other dependencies (including Gradle itself) are automatically downloaded. It's all about automation at the cost of just 70KB.

Installing Gradle manually and then calling the wrapper is possible. I did this for years and I had a rather negative experience with that approach. It is likely to introduce problems that haven't been addressed in the discussion. Not long ago, I got tons of questions related to Could not determine java version from ‘9.0.1’ and similar problems. I learned my lesson ;)

Anyway, if you are still not convinced, just throw those files away and rebase the things you like into master. For me this is not a big deal. I will keep the wrapper in our fork since it works great for us.

distributionPath=wrapper/dists
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-4.5.1-all.zip
172 changes: 172 additions & 0 deletions gradlew
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

84 changes: 84 additions & 0 deletions gradlew.bat
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions settings.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
rootProject.name = 'apted'
Copy link
Copy Markdown
Collaborator

@mateuszpawlik mateuszpawlik Feb 12, 2018

Choose a reason for hiding this comment

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

The file has not changed. This should not be part of the commit.

Copy link
Copy Markdown
Author

@miho miho Feb 12, 2018

Choose a reason for hiding this comment

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

This file is new. It is the proper way to name a gradle project. What do you mean by 'it has not changed'?

Copy link
Copy Markdown
Collaborator

@mateuszpawlik mateuszpawlik Feb 13, 2018

Choose a reason for hiding this comment

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

Again my mistake. The documentation has changed heavily since I read it. This file is fine.

4 changes: 2 additions & 2 deletions src/main/java/costmodel/CostModel.java → ...nisalzburg/apted/costmodel/CostModel.java
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@
* SOFTWARE.
*/

package costmodel;
package at.unisalzburg.apted.costmodel;

import node.Node;
import at.unisalzburg.apted.node.Node;

/**
* This interface specifies the methods to implement for a custom cost model.
Expand Down
10 changes: 5 additions & 5 deletions ...EditOperationStringNodeDataCostModel.java → ...EditOperationStringNodeDataCostModel.java
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,14 @@
* SOFTWARE.
*/

package costmodel;
package at.unisalzburg.apted.costmodel;

import costmodel.CostModel;
import node.Node;
import node.StringNodeData;
import at.unisalzburg.apted.costmodel.CostModel;
import at.unisalzburg.apted.node.Node;
import at.unisalzburg.apted.node.StringNodeData;

/**
* This is a cost model defined on {@link node.StringNodeData} with a fixed cost
* This is a cost model defined on {@link at.unisalzburg.apted.node.StringNodeData} with a fixed cost
* per edit operation.
*/
public class PerEditOperationStringNodeDataCostModel implements CostModel<StringNodeData> {
Expand Down
8 changes: 4 additions & 4 deletions ...n/java/costmodel/StringUnitCostModel.java → .../apted/costmodel/StringUnitCostModel.java
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@
* SOFTWARE.
*/

package costmodel;
package at.unisalzburg.apted.costmodel;

import costmodel.CostModel;
import node.Node;
import node.StringNodeData;
import at.unisalzburg.apted.costmodel.CostModel;
import at.unisalzburg.apted.node.Node;
import at.unisalzburg.apted.node.StringNodeData;

/**
* This is a unit-nost model defined on string labels.
Expand Down
Loading