fail build on wildcard imports

[rmuir]

Wildcard imports are terrible, they cause ambiguity in the code,
make it not compile with the future versions of java in many cases.

We should simply fail the build on this, it is messiness, caused by
messy Intellij configuration

[ywelsch]

+1

[jpountz]

+1

[jasontedor]

I am very much in favor in this change, but this will also require a change to our CONTRIBUTING.md where the lines

Don't worry too much about imports. Try not to change the order but don't worry about fighting your IDE to stop it from switching from * imports to specific imports or from specific to * imports.

currently appear.

Additionally, this very topic came up a few months ago so I think we should ensure we have consensus here.

[rmuir]

I expect Intellij users to defend their shitty IDE, but they have no ground to stand on.

Bottom line: wildcard imports are bad.

[jasontedor]

I expect Intellij users to defend their shitty IDE, but they have no ground to stand on.

The issue is independent of the IDE (and it's a simple configuration change in IntelliJ).

[rjernst]

+1 to the change as is.

I think we can followup separately with a change to the contributing docs. This is for master only.

[rmuir]

I see wildcard imports being created by intellij ides, not folks using emacs or eclipse.

Its fine to keep the settings in intellij for wildcard imports if you like that. just clean up your mess before pushing, before pushing it onto others to deal with.

[jpountz]

I think we can followup separately with a change to the contributing docs. This is for master only.

I don't understand the push back. I think @jasontedor 's note makes perfect sense: we should be consistent about what we tell contributors and what we enforce. If this was a burden that was in the way of a simple incremental change, I would have a different opinion, but here it's nothing like that?

[jasontedor]

+1 to the change as is.

In addition to @jpountz's reply, it can not go in as is because the build will fail. It will require a wholesale change to imports in all source files.

[bleskes]

+1 to every one on this thread - both getting this in and doing it properly, with a doc change and preventing the build from immediately failing.

On 11 Dec 2015, at 17:54, Jason Tedor notifications@github.com wrote:

+1 to the change as is.

In addition to @jpountz's reply, it can not go in as is because the build will fail. It will require a wholesale change to imports in all source files.

—
Reply to this email directly or view it on GitHub.

[nik9000]

In addition to @jpountz's reply, it can not go in as is because the build will fail. It will require a wholesale change to imports in all source files.

I'm up for taking 1/2 of the classes this evening and just opening them in Eclipse, organizing imports, and saving them again. Its like what I did with Map only it won't take 123187123 hours.

[jpountz]

@nik9000 I suspect some ides have a way to do it automatically for a whole project, maybe it's not such a painful task?

[nik9000]

It'll make merge conflicts. But having Eclipse do no * imports and intellij do them after 5 imports or something has been like a merge conflict machine for years.

[jasontedor]

I'm up for taking 1/2 of the classes this evening and just opening them in Eclipse, organizing imports, and saving them again. Its like what I did with Map only it won't take 123187123 hours.

IntelliJ can do it in two clicks (do not even need to open the files, can wholesale optimize the entire project).

[nik9000]

IntelliJ can do it in two clicks (do not even need to open the files, can wholesale optimize the entire project).

Go nuts. I dunno if I have such a button but I'd certainly look for it.

[nik9000]

Go nuts. I dunno if I have such a button but I'd certainly look for it.

Looks like Eclipse has it in 4 steps.

1. Click in file list.
2. Ctrl-A
3. Click Source
4. Click Organize Imports

[nik9000]

Ok - I just finished doing it on whatever I had checked out. The diff is an unreviewable 1.4MB.

Proposal: We make two PRs. One with all the * imports removed which we won't review at all and just cram in if the tests pass. The other we'll actually review. This one can be the first one. We can get fancy and merge this one into the big one and then merge them at the same time or something so its atomic.

[dakrone]

+1

[rjernst]

I think we can followup separately with a change to the contributing docs. This is for master only.

I don't understand the push back.

My push back is to making changes that are good sit and wait on bikeshedding. And I am confident any change to contributing docs about IDE setup would be bikeshedded (wording, particular path to change settings, particular value to this setting in intellij because it is a number of classes before using wildcard). But this is a general problem: master has to be more fluid. Should we have a followup issue to update contributing documentation? Absolutely. But should this change sit for 2 days while devs from 2 continents argue about wording that doesn't affect the actual change?

it can not go in as is because the build will fail

I had assumed this would be done just before merge, with the couple of clicks mentioned here already.

[nik9000]

I had assumed this would be done just before merge, with the couple of clicks mentioned here already.

That'd be fine too. Just for reference - the project didn't build after I ran the cleanup. I'm sure it was minor but I just reverted and went back to what I was working on. So it'll take a little bit of by hand cleanup. Probably not much. Its fine.

But this is a general problem: master has to be more fluid.

I'm super forgetful and, as someone who relatively recently made this my full time job I remember very clearly what it was like when it wasn't. Its alienating when the contributing docs are wrong. So I'm in the "just change the wording now and bike shed about the wording in another PR". So I sent @rmuir a PR against his branch with an amended CONTRIBUTING.md.

[nik9000]

LGTM now. Thanks for fixing the typo.

[jpountz]

👍