Skip to content
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

Jump to bottom

Add support for javac-style @params files #77

Closed
wants to merge 1 commit into from
Closed

Add support for javac-style @params files #77

wants to merge 1 commit into from

Conversation

sormuras
Copy link
Contributor

@sormuras sormuras commented Sep 29, 2016 •
edited
Loading

This PR adds support for javac-style @params files.

Example: @params.txt:

--dry-run
--set-exit-if-changed
@@-escaped-option-starting-with-a-single-@-character

α.java
β.java
Ω.java

@nested-params-file

This solves two problems with current command line program:

@lowasser
Copy link
Contributor

If I were doing this on a Linux box I'd just write cat file | xargs run-formatter. Is there not something equivalent in your environment?

@sormuras
Copy link
Contributor Author

sormuras commented Sep 30, 2016 •
edited
Loading

Maybe. I think, not.

The main problem is the command line / shell environment on Windows. If those programs you mention exists on Windows, they'll still use the non-Unicode ... shell thing. If I'm not missing something, it would still not work.

See the MainTest#testMainWithUnicodeArgumentFailsOnWindows included in this PR:

    String expected = "ℕ.java";
    String actual = "N.java"

And this is a good (human-readable) guess -- almost all other Unicode characters are translated to ?s.

jbduncan
jbduncan reviewed Sep 30, 2016
View reviewed changes
core/src/main/java/com/google/googlejavaformat/java/CommandLineOptionsParser.java Outdated
Path path = Paths.get(value);
try {
for (String line : Files.readAllLines(path, StandardCharsets.UTF_8)) {
line = line.trim();
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this line would be less bug-prone if it were replaced with CharMatcher.whitespace().trimFrom(line).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced.

@cushon
Copy link
Collaborator

cushon commented Oct 12, 2017

What do you think about supporting javac-style @params files, instead of doing this specifically for the sources argument?

@sormuras
Copy link
Contributor Author

In progress...

@cushon
Copy link
Collaborator

cushon commented Oct 12, 2017

Maybe something like this:
https://github.com/google/turbine/blob/a5453051f0c8b71a833f6dc73caaea43bd24a0db/java/com/google/turbine/options/TurbineOptionsParser.java#L62

@sormuras
Copy link
Contributor Author

Ah, with escaping. Check.

@sormuras
Copy link
Contributor Author

Expand the "args" in Main#processArgs(String...) or CommandLineOptionsParser#parse(Iterable<String>)?

I go with the second option for an initial PR.

@sormuras sormuras changed the title Command line option sourcesFile added Add support for javac-style @params files Oct 12, 2017
@sormuras
Copy link
Contributor Author

sormuras commented Oct 12, 2017 •
edited
Loading

Ready for review. Not. Missed the now out-dated UsageException implementation...

cushon
cushon reviewed Oct 12, 2017
View reviewed changes
Copy link
Collaborator

@cushon cushon left a comment

Choose a reason for hiding this comment

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

LGTM aside from a couple of minor questions.

core/src/test/java/com/google/googlejavaformat/java/MainTest.java Outdated
@@ -119,6 +119,37 @@ public void testMain() throws Exception {
assertThat(process.exitValue()).isEqualTo(0);
}

@Test
// https://stackoverflow.com/questions/7660651/passing-command-line-unicode-argument-to-java-code
public void testMainWithUnicodeArgumentFailsOnWindows() throws Exception {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you want to include this test, or was it more to document the problem? I guess it would provide some notice if this changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It only demonstrates the underlying issue why I needed that --sourceFile feature.

I think, it's superseded by now and I'll remove it from this PR.

core/src/test/java/com/google/googlejavaformat/java/CommandLineOptionsParserTest.java Outdated
CommandLineOptions options =
CommandLineOptionsParser.parse(Arrays.asList("@" + path.toString()));
assertThat(options.files()).containsExactly("Hello.java", "ℕ.java", "Goodbye.java");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also test non-params file arguments before/after the params file, the @@-escape syntax, and maybe a nested params file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

@sormuras
Copy link
Contributor Author

sormuras commented Oct 12, 2017 •
edited
Loading

Should we also support comment lines? Meaning ignore lines that start with //.

@sormuras
Copy link
Contributor Author

Almost down to a single feature branch. :)

@cushon cushon closed this in 598f248 Oct 14, 2017
@sormuras sormuras deleted the sourcesFile branch October 14, 2017 03:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cushon cushon cushon left review comments

@jbduncan jbduncan jbduncan left review comments

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sormuras @lowasser @cushon @jbduncan @googlebot