You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@netbeans.apache.org by matthiasblaesing <gi...@git.apache.org> on 2017/09/24 22:36:24 UTC
[GitHub] incubator-netbeans pull request #6: [NETBEANS-54] Module Review db
GitHub user matthiasblaesing opened a pull request:
https://github.com/apache/incubator-netbeans/pull/6
[NETBEANS-54] Module Review db
- external library derby-10.2.2.0.jar: ALv2 licensed, notice
added, Maven coordinates added.
- checked Rat report; missing license headerd added,
unrecognized license headers manually changed, ignored
l10n.list, manifest.mf, org-netbeans-modules-db.sig and
*.form (see central problems)
- skimmed through the module, did not notice additional problems
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/matthiasblaesing/incubator-netbeans db-review
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-netbeans/pull/6.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #6
----
commit ac64007efbf2f6fd3383e1aa5c3c4fa066f0273b
Author: Matthias Bläsing <mb...@doppel-helix.eu>
Date: 2017-09-24T18:55:43Z
[NETBEANS-54] Module Review db
- external library derby-10.2.2.0.jar: ALv2 licensed, notice
added, Maven coordinates added.
- checked Rat report; missing license headerd added,
unrecognized license headers manually changed, ignored
l10n.list, manifest.mf, org-netbeans-modules-db.sig and
*.form (see central problems)
- skimmed through the module, did not notice additional problems
----
---
[GitHub] incubator-netbeans pull request #6: [NETBEANS-54] Module Review db
Posted by jlahoda <gi...@git.apache.org>.
Github user jlahoda commented on a diff in the pull request:
https://github.com/apache/incubator-netbeans/pull/6#discussion_r141172728
--- Diff: db/test/unit/src/org/netbeans/modules/db/explorer/DbDriverManagerTest.java ---
@@ -221,7 +221,7 @@ private static JDBCDriver createJDBCDriver() {
private static JDBCDriver createDummyJDBCDriver(File dataDir) throws MalformedURLException {
URL url = dataDir.toURL();
- return JDBCDriver.create("test_driver", "DbDriverManagerTest DummyDriver", "DummyDriver", new URL[] { url });
+ return JDBCDriver.create("test_driver", "DbDriverManagerTest DummyDriver", "org.netbeans.modules.db.explorer.DbDriverManagerTest$DriverImpl", new URL[] { url });
--- End diff --
If I were doing this, I'd probably do two commits - one license-related cleanup, and one test fix/cleanup. OTOH, I don't think this is important enough to block the change on it, so I'll leave the decision on you on this matter.
---
[GitHub] incubator-netbeans issue #6: [NETBEANS-54] Module Review db
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/incubator-netbeans/pull/6
Can one of the admins verify this patch?
---
[GitHub] incubator-netbeans pull request #6: [NETBEANS-54] Module Review db
Posted by matthiasblaesing <gi...@git.apache.org>.
Github user matthiasblaesing commented on a diff in the pull request:
https://github.com/apache/incubator-netbeans/pull/6#discussion_r140853623
--- Diff: db/external/binaries-list ---
@@ -1,2 +1,18 @@
-F787C9B484CD7526F866C21D8925C4DACE467F8A derby-10.2.2.0.jar
-97771BE04E7452FC197EB875D2591A7E91F274D0 derby-10.2.2.0.zip
--- End diff --
I did not find any usage and as such I removed that reference.
---
[GitHub] incubator-netbeans pull request #6: [NETBEANS-54] Module Review db
Posted by jlahoda <gi...@git.apache.org>.
Github user jlahoda commented on a diff in the pull request:
https://github.com/apache/incubator-netbeans/pull/6#discussion_r141121602
--- Diff: db/test/unit/src/org/netbeans/modules/db/explorer/DbDriverManagerTest.java ---
@@ -221,7 +221,7 @@ private static JDBCDriver createJDBCDriver() {
private static JDBCDriver createDummyJDBCDriver(File dataDir) throws MalformedURLException {
URL url = dataDir.toURL();
- return JDBCDriver.create("test_driver", "DbDriverManagerTest DummyDriver", "DummyDriver", new URL[] { url });
+ return JDBCDriver.create("test_driver", "DbDriverManagerTest DummyDriver", "org.netbeans.modules.db.explorer.DbDriverManagerTest$DriverImpl", new URL[] { url });
--- End diff --
So, the test was passing before other changes in the pull request and started to fail after the changes have been made?
---
[GitHub] incubator-netbeans pull request #6: [NETBEANS-54] Module Review db
Posted by jlahoda <gi...@git.apache.org>.
Github user jlahoda commented on a diff in the pull request:
https://github.com/apache/incubator-netbeans/pull/6#discussion_r140850184
--- Diff: db/external/binaries-list ---
@@ -1,2 +1,18 @@
-F787C9B484CD7526F866C21D8925C4DACE467F8A derby-10.2.2.0.jar
-97771BE04E7452FC197EB875D2591A7E91F274D0 derby-10.2.2.0.zip
--- End diff --
The zip is unused, right? (I did a quick search, and didn't find a place where it would be used.)
---
[GitHub] incubator-netbeans pull request #6: [NETBEANS-54] Module Review db
Posted by matthiasblaesing <gi...@git.apache.org>.
Github user matthiasblaesing commented on a diff in the pull request:
https://github.com/apache/incubator-netbeans/pull/6#discussion_r140854981
--- Diff: db/test/unit/src/org/netbeans/modules/db/explorer/DbDriverManagerTest.java ---
@@ -221,7 +221,7 @@ private static JDBCDriver createJDBCDriver() {
private static JDBCDriver createDummyJDBCDriver(File dataDir) throws MalformedURLException {
URL url = dataDir.toURL();
- return JDBCDriver.create("test_driver", "DbDriverManagerTest DummyDriver", "DummyDriver", new URL[] { url });
+ return JDBCDriver.create("test_driver", "DbDriverManagerTest DummyDriver", "org.netbeans.modules.db.explorer.DbDriverManagerTest$DriverImpl", new URL[] { url });
--- End diff --
It is related, as I run unittests and they failed. This was necessary to let them cleanly again. This is related to the change in the binaries-list, as the binaries are used by the unittests.
---
[GitHub] incubator-netbeans pull request #6: [NETBEANS-54] Module Review db
Posted by jlahoda <gi...@git.apache.org>.
Github user jlahoda commented on a diff in the pull request:
https://github.com/apache/incubator-netbeans/pull/6#discussion_r140850999
--- Diff: db/test/unit/src/org/netbeans/modules/db/explorer/DbDriverManagerTest.java ---
@@ -221,7 +221,7 @@ private static JDBCDriver createJDBCDriver() {
private static JDBCDriver createDummyJDBCDriver(File dataDir) throws MalformedURLException {
URL url = dataDir.toURL();
- return JDBCDriver.create("test_driver", "DbDriverManagerTest DummyDriver", "DummyDriver", new URL[] { url });
+ return JDBCDriver.create("test_driver", "DbDriverManagerTest DummyDriver", "org.netbeans.modules.db.explorer.DbDriverManagerTest$DriverImpl", new URL[] { url });
--- End diff --
So what is the significance of this change? (If it is unrelated to "license" review, then I'd suggest to send it separately, for clarity.)
---
[GitHub] incubator-netbeans pull request #6: [NETBEANS-54] Module Review db
Posted by matthiasblaesing <gi...@git.apache.org>.
Github user matthiasblaesing closed the pull request at:
https://github.com/apache/incubator-netbeans/pull/6
---
[GitHub] incubator-netbeans pull request #6: [NETBEANS-54] Module Review db
Posted by matthiasblaesing <gi...@git.apache.org>.
Github user matthiasblaesing commented on a diff in the pull request:
https://github.com/apache/incubator-netbeans/pull/6#discussion_r141124129
--- Diff: db/test/unit/src/org/netbeans/modules/db/explorer/DbDriverManagerTest.java ---
@@ -221,7 +221,7 @@ private static JDBCDriver createJDBCDriver() {
private static JDBCDriver createDummyJDBCDriver(File dataDir) throws MalformedURLException {
URL url = dataDir.toURL();
- return JDBCDriver.create("test_driver", "DbDriverManagerTest DummyDriver", "DummyDriver", new URL[] { url });
+ return JDBCDriver.create("test_driver", "DbDriverManagerTest DummyDriver", "org.netbeans.modules.db.explorer.DbDriverManagerTest$DriverImpl", new URL[] { url });
--- End diff --
No - master fails this test also.
---
[GitHub] incubator-netbeans issue #6: [NETBEANS-54] Module Review db
Posted by matthiasblaesing <gi...@git.apache.org>.
Github user matthiasblaesing commented on the issue:
https://github.com/apache/incubator-netbeans/pull/6
@svenreimers @jlahoda thanks for taking time for review!
I merged the change into master. I'm manually closing this here, as the merge was done as a fast-forward merge and so no merge commit was created.
---
[GitHub] incubator-netbeans issue #6: [NETBEANS-54] Module Review db
Posted by matthiasblaesing <gi...@git.apache.org>.
Github user matthiasblaesing commented on the issue:
https://github.com/apache/incubator-netbeans/pull/6
@svenreimers @jlahoda I reverted the fix for the unittest to get this merged at some point, but I don't agree with your assessment.
---
Re: [GitHub] incubator-netbeans issue #6: [NETBEANS-54] Module Review db
Posted by Geertjan Wielenga <ge...@googlemail.com>.
Indeed, if there's been discussion about a PR and if there's agreement on
it (and no big disagreements) and if you're a comitter... go ahead and do
the push.
Gj
On Wed, 27 Sep 2017 at 20:54, jlahoda <gi...@git.apache.org> wrote:
> Github user jlahoda commented on the issue:
>
> https://github.com/apache/incubator-netbeans/pull/6
>
> Matthias, you are an Apache Netbeans committer, right? So you should
> be able to push this directly to the apache repository (
> https://git-wip-us.apache.org/repos/asf/incubator-netbeans). (I would
> suggest to rebase the changes before merging so that there's no actual
> merge commit, but up to you, I am not a git expert.) Please let me know if
> you need any help.
>
> Thanks for working on this!
>
>
> ---
>
[GitHub] incubator-netbeans issue #6: [NETBEANS-54] Module Review db
Posted by jlahoda <gi...@git.apache.org>.
Github user jlahoda commented on the issue:
https://github.com/apache/incubator-netbeans/pull/6
Matthias, you are an Apache Netbeans committer, right? So you should be able to push this directly to the apache repository (https://git-wip-us.apache.org/repos/asf/incubator-netbeans). (I would suggest to rebase the changes before merging so that there's no actual merge commit, but up to you, I am not a git expert.) Please let me know if you need any help.
Thanks for working on this!
---
[GitHub] incubator-netbeans pull request #6: [NETBEANS-54] Module Review db
Posted by jlahoda <gi...@git.apache.org>.
Github user jlahoda commented on a diff in the pull request:
https://github.com/apache/incubator-netbeans/pull/6#discussion_r140850531
--- Diff: db/external/derby-10.2.2.0-notice.txt ---
@@ -0,0 +1,298 @@
+=========================================================================
--- End diff --
Not quite clear to me how the notice should look like - I think this is OK for now, we will learn if there's an issue with it.
---
[GitHub] incubator-netbeans pull request #6: [NETBEANS-54] Module Review db
Posted by svenreimers <gi...@git.apache.org>.
Github user svenreimers commented on a diff in the pull request:
https://github.com/apache/incubator-netbeans/pull/6#discussion_r141175203
--- Diff: db/test/unit/src/org/netbeans/modules/db/explorer/DbDriverManagerTest.java ---
@@ -221,7 +221,7 @@ private static JDBCDriver createJDBCDriver() {
private static JDBCDriver createDummyJDBCDriver(File dataDir) throws MalformedURLException {
URL url = dataDir.toURL();
- return JDBCDriver.create("test_driver", "DbDriverManagerTest DummyDriver", "DummyDriver", new URL[] { url });
+ return JDBCDriver.create("test_driver", "DbDriverManagerTest DummyDriver", "org.netbeans.modules.db.explorer.DbDriverManagerTest$DriverImpl", new URL[] { url });
--- End diff --
Same here. I would prefer a split - first ee fix sll the thing eith regards to licensing etc. stuff and afterwards we start fixing failing tests (I assume we will find more)
---