You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by schaumb <gi...@git.apache.org> on 2017/06/09 09:22:20 UTC
[GitHub] commons-cli pull request #12: [CLI-274] implement EXISTING_FILE_VALUE type h...
GitHub user schaumb opened a pull request:
https://github.com/apache/commons-cli/pull/12
[CLI-274] implement EXISTING_FILE_VALUE type handler
when the user pass option type FileInputStream.class, I think the expected behavior for the return value is the same type, which the user passed.
Before this there was no check whether the file exist.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/schaumb/commons-cli patch-1
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/commons-cli/pull/12.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 #12
----
commit abfcc8211f529ab75f3b3edd4a827e484109eb0b
Author: Bela Schaum <sc...@users.noreply.github.com>
Date: 2017-04-17T11:45:09Z
implement EXISTING_FILE_VALUE type handler
when the user pass option type FileInputStream.class, I think the expected behavior for the return value is the same type, which the user passed.
Before this there was no check whether the file exist.
commit a315e187908d184b1d40f9e7425bc65333a2d07a
Author: Bela Schaum <sc...@users.noreply.github.com>
Date: 2017-04-17T14:03:11Z
[CLI-274] misseed import
commit 9039cbd454346276c632ae2424a3c20e18a2d276
Author: Bela Schaum <sc...@users.noreply.github.com>
Date: 2017-04-17T14:21:42Z
Update PatternOptionBuilderTest.java
commit c1cbe9dd69f80b627ea046095af842bfafb15803
Author: Bela Schaum <sc...@users.noreply.github.com>
Date: 2017-04-17T14:27:45Z
Update PatternOptionBuilderTest.java
commit e3d65b0ee3b08599bd787a7721afc326d1bcccf1
Author: Bela Schaum <sc...@users.noreply.github.com>
Date: 2017-04-17T14:33:42Z
Update PatternOptionBuilderTest.java
-.-' sry
commit fac33304c67496380cd168d71cac79dbc0e60142
Author: Bela Schaum <sc...@users.noreply.github.com>
Date: 2017-04-17T14:39:15Z
Update PatternOptionBuilderTest.java
really exist file name?
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-cli issue #12: [CLI-274] implement EXISTING_FILE_VALUE type handler
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/commons-cli/pull/12
[](https://coveralls.io/builds/11947210)
Coverage increased (+0.009%) to 96.244% when pulling **f4a28c0463a414464ebe214a7790fde0b0069e3e on schaumb:patch-1** into **70a392756c713f404fed0e3ddd48aa18ce20485f on apache:master**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-cli pull request #12: [CLI-274] implement EXISTING_FILE_VALUE type h...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/commons-cli/pull/12
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-cli pull request #12: [CLI-274] implement EXISTING_FILE_VALUE type h...
Posted by schaumb <gi...@git.apache.org>.
Github user schaumb commented on a diff in the pull request:
https://github.com/apache/commons-cli/pull/12#discussion_r121406356
--- Diff: src/test/java/org/apache/commons/cli/PatternOptionBuilderTest.java ---
@@ -159,13 +161,12 @@ public void testURLPattern() throws Exception
@Test
public void testExistingFilePattern() throws Exception
{
- final Options options = PatternOptionBuilder.parsePattern("f<");
+ final Options options = PatternOptionBuilder.parsePattern("f<g<");
final CommandLineParser parser = new PosixParser();
- final CommandLine line = parser.parse(options, new String[] { "-f", "test.properties" });
-
- assertEquals("f value", new File("test.properties"), line.getOptionObject("f"));
-
- // todo test if an error is returned if the file doesn't exists (when it's implemented)
+ final CommandLine line = parser.parse(options, new String[] { "-f", "test.properties", "-g", "/dev/null" });
+
+ assertNotNull("option g not parsed, or not FileInputStream", (FileInputStream) line.getOptionObject("g"));
+ assertNull("option f parsed", (FileInputStream) line.getOptionObject("f"));
--- End diff --
So basically `<` means this parameter is an existing file parameter.
When I built the options manually, the type is what I need to set is `PatternOptionBuilder.EXISTING_FILE_VALUE`, which is `FileInputStream`.
This is why think `line.getOptionObject("f")` need to return an object of type `FileInputStream`.
Yes, It is null because test.properties does not exist, or not readable. This can be made clearer somehow.
Another task is to find a cross-platform file which is exist (and not /dev/null).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-cli pull request #12: [CLI-274] implement EXISTING_FILE_VALUE type h...
Posted by schaumb <gi...@git.apache.org>.
Github user schaumb commented on a diff in the pull request:
https://github.com/apache/commons-cli/pull/12#discussion_r121654518
--- Diff: src/test/java/org/apache/commons/cli/PatternOptionBuilderTest.java ---
@@ -159,13 +161,15 @@ public void testURLPattern() throws Exception
@Test
public void testExistingFilePattern() throws Exception
{
- final Options options = PatternOptionBuilder.parsePattern("f<");
+ final Options options = PatternOptionBuilder.parsePattern("f<g<");
final CommandLineParser parser = new PosixParser();
- final CommandLine line = parser.parse(options, new String[] { "-f", "test.properties" });
+ final CommandLine line = parser.parse(options, new String[] { "-f", "non-existing.file", "-g", "src/test/resources/existing-readable.file" });
+
+ assertNull("option f parsed", line.getOptionObject("f"));
- assertEquals("f value", new File("test.properties"), line.getOptionObject("f"));
--- End diff --
the 168. line tests f parameter
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-cli pull request #12: [CLI-274] implement EXISTING_FILE_VALUE type h...
Posted by britter <gi...@git.apache.org>.
Github user britter commented on a diff in the pull request:
https://github.com/apache/commons-cli/pull/12#discussion_r121589022
--- Diff: src/test/java/org/apache/commons/cli/PatternOptionBuilderTest.java ---
@@ -159,13 +161,15 @@ public void testURLPattern() throws Exception
@Test
public void testExistingFilePattern() throws Exception
{
- final Options options = PatternOptionBuilder.parsePattern("f<");
+ final Options options = PatternOptionBuilder.parsePattern("f<g<");
final CommandLineParser parser = new PosixParser();
- final CommandLine line = parser.parse(options, new String[] { "-f", "test.properties" });
+ final CommandLine line = parser.parse(options, new String[] { "-f", "non-existing.file", "-g", "src/test/resources/existing-readable.file" });
+
+ assertNull("option f parsed", line.getOptionObject("f"));
- assertEquals("f value", new File("test.properties"), line.getOptionObject("f"));
-
- // todo test if an error is returned if the file doesn't exists (when it's implemented)
+ Object parsedReadableFileStream = line.getOptionObject("g");
+ assertNotNull("option g not parsed", parsedReadableFileStream);
+ assertEquals("option g not FileInputStream", FileInputStream.class, parsedReadableFileStream.getClass());
--- End diff --
How about `assertTrue(parsedReadableFileStream instanceof FileInputStream)`? Your assert will fail for subclasses of `FileInputStream`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-cli pull request #12: [CLI-274] implement EXISTING_FILE_VALUE type h...
Posted by britter <gi...@git.apache.org>.
Github user britter commented on a diff in the pull request:
https://github.com/apache/commons-cli/pull/12#discussion_r121290023
--- Diff: src/test/java/org/apache/commons/cli/PatternOptionBuilderTest.java ---
@@ -159,13 +161,12 @@ public void testURLPattern() throws Exception
@Test
public void testExistingFilePattern() throws Exception
{
- final Options options = PatternOptionBuilder.parsePattern("f<");
+ final Options options = PatternOptionBuilder.parsePattern("f<g<");
final CommandLineParser parser = new PosixParser();
- final CommandLine line = parser.parse(options, new String[] { "-f", "test.properties" });
-
- assertEquals("f value", new File("test.properties"), line.getOptionObject("f"));
-
- // todo test if an error is returned if the file doesn't exists (when it's implemented)
+ final CommandLine line = parser.parse(options, new String[] { "-f", "test.properties", "-g", "/dev/null" });
+
+ assertNotNull("option g not parsed, or not FileInputStream", (FileInputStream) line.getOptionObject("g"));
+ assertNull("option f parsed", (FileInputStream) line.getOptionObject("f"));
--- End diff --
I don't understand this assertion. Shouldn't `line.getOptionObject("f")` return an object of type `File`? And why is it null? because `test.properties` does not exist?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-cli pull request #12: [CLI-274] implement EXISTING_FILE_VALUE type h...
Posted by britter <gi...@git.apache.org>.
Github user britter commented on a diff in the pull request:
https://github.com/apache/commons-cli/pull/12#discussion_r121493262
--- Diff: src/test/java/org/apache/commons/cli/PatternOptionBuilderTest.java ---
@@ -159,13 +161,12 @@ public void testURLPattern() throws Exception
@Test
public void testExistingFilePattern() throws Exception
{
- final Options options = PatternOptionBuilder.parsePattern("f<");
+ final Options options = PatternOptionBuilder.parsePattern("f<g<");
final CommandLineParser parser = new PosixParser();
- final CommandLine line = parser.parse(options, new String[] { "-f", "test.properties" });
-
- assertEquals("f value", new File("test.properties"), line.getOptionObject("f"));
-
- // todo test if an error is returned if the file doesn't exists (when it's implemented)
+ final CommandLine line = parser.parse(options, new String[] { "-f", "test.properties", "-g", "/dev/null" });
+
+ assertNotNull("option g not parsed, or not FileInputStream", (FileInputStream) line.getOptionObject("g"));
+ assertNull("option f parsed", (FileInputStream) line.getOptionObject("f"));
--- End diff --
> Yes, It is null because test.properties does not exist, or not readable. This can be made clearer somehow.
Maybe by calling it `non-existing.file`?
> Another task is to find a cross-platform file which is exist (and not /dev/null).
You could put add dummy file in `src/test/resources` and use that one.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-cli pull request #12: [CLI-274] implement EXISTING_FILE_VALUE type h...
Posted by britter <gi...@git.apache.org>.
Github user britter commented on a diff in the pull request:
https://github.com/apache/commons-cli/pull/12#discussion_r121588869
--- Diff: src/test/java/org/apache/commons/cli/PatternOptionBuilderTest.java ---
@@ -159,13 +161,15 @@ public void testURLPattern() throws Exception
@Test
public void testExistingFilePattern() throws Exception
{
- final Options options = PatternOptionBuilder.parsePattern("f<");
+ final Options options = PatternOptionBuilder.parsePattern("f<g<");
final CommandLineParser parser = new PosixParser();
- final CommandLine line = parser.parse(options, new String[] { "-f", "test.properties" });
+ final CommandLine line = parser.parse(options, new String[] { "-f", "non-existing.file", "-g", "src/test/resources/existing-readable.file" });
+
+ assertNull("option f parsed", line.getOptionObject("f"));
- assertEquals("f value", new File("test.properties"), line.getOptionObject("f"));
--- End diff --
Now we're not testing the `-f` option any more...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-cli issue #12: [CLI-274] implement EXISTING_FILE_VALUE type handler
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/commons-cli/pull/12
[](https://coveralls.io/builds/11939016)
Coverage increased (+0.009%) to 96.244% when pulling **4f9c95bcb246b64f7f6756cc3840d2061a262fe7 on schaumb:patch-1** into **70a392756c713f404fed0e3ddd48aa18ce20485f on apache:master**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-cli issue #12: [CLI-274] implement EXISTING_FILE_VALUE type handler
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/commons-cli/pull/12
[](https://coveralls.io/builds/11938473)
Coverage increased (+0.009%) to 96.244% when pulling **1e59d0c2fd1cfee450d0104734307306803a84e0 on schaumb:patch-1** into **70a392756c713f404fed0e3ddd48aa18ce20485f on apache:master**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-cli pull request #12: [CLI-274] implement EXISTING_FILE_VALUE type h...
Posted by britter <gi...@git.apache.org>.
Github user britter commented on a diff in the pull request:
https://github.com/apache/commons-cli/pull/12#discussion_r121589195
--- Diff: src/test/java/org/apache/commons/cli/PatternOptionBuilderTest.java ---
@@ -159,13 +161,15 @@ public void testURLPattern() throws Exception
@Test
public void testExistingFilePattern() throws Exception
{
- final Options options = PatternOptionBuilder.parsePattern("f<");
+ final Options options = PatternOptionBuilder.parsePattern("f<g<");
--- End diff --
Looking at this, I think we're doing to many things at once in this test. Why don't we split this test up into to separate tests? One for `f<` and one for `g<`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-cli issue #12: [CLI-274] implement EXISTING_FILE_VALUE type handler
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/commons-cli/pull/12
[](https://coveralls.io/builds/11900307)
Coverage increased (+0.009%) to 96.244% when pulling **aea58f8677e55513ae281c49b91a3abce5ee7d1b on schaumb:patch-1** into **70a392756c713f404fed0e3ddd48aa18ce20485f on apache:master**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org