Import cleanup

[bjornharrtell]

This is the first, and largest, of the patches needed for autotranslation to JavaScript (see bjornharrtell/jsts#178) and aims to remove imports that are unused and resolve on demand imports.

[bjornharrtell]

The imports organization is automated using Eclipse, so it's easy to update this PR later if it's not a good time to merge. But it might be nice to have this in before the move to LocationTech?

[jnh5y]

Hi @bjornharrtell, a few of who have projects which depend on JTS have joined Martin to help with PRs and general project management as JTS moves to LocationTech.

The Eclipse Foundation and LocationTech are very thorough about keeping track of who contributes to a project and ensuring that a little paperwork is on file. In particular, to contribute to an Eclipse/LT project, you'll need to fill out a Contributor License Agreement. More info is here: http://www.eclipse.org/legal/CLA.php.

As a note, the email address you use for GitHub should be the one you use when creating your Eclipse account. After a day or two, you'll be able to verify that your CLA is on file here: https://projects.eclipse.org/user/cla/validate.

As one additional detail, contributor are required to use the Git sign-off flag to indicate that their work is their own and can be contributed. This is done by adding the '-s' flag to the 'git commit' command.

At the moment, lots of changes are happening as JTS moves to LocationTech. Merging PRs is Martin's decision, but I'd suggest that we hold off until the repo is moved over to LocationTech. This should be happening in the next week or so. (If it goes longer than that, we can discuss things again.)

Other than that, Martin mentioned the JTS to JavaScript project, and Jody, Rob, and I are interested in figuring out how to support you as best as possible.

Is your project which does the translation available on GitHub?

[lossyrob]

Question: Is this a request to no longer use wildcard imports in JTS? This PR seems to remove that pattern, and I'm wondering if this means we either A. enforce no unused imports or wildcard imports in the future or B. have PRs coming in constantly fixing wildcard/unused. What is the reason that the translation can't handle this?

[dr-jts]

@jnh5y SInce this is really just code cleanup, rather than net new code, would it be possible to accept this without a CLA? Alternatively, it should be easy for me to make these changes and submit.

@lossyrob re the removal of wildcard imports - while I actually prefer wildcards, Eclipse Organize Imports always changes them to explicit ones, so it seems that is a best practice for some reason? I think it might be able to set this up as an automatic change on save, so this should not be too onerous to maintain going forward.

[jnh5y]

@dr-jts If this is the kind of formatting the Eclipse can automatically do, redoing it seems sensible. Signing a CLA takes 5 minutes and covers the project forever, so I'd really suggest we get in the habit of requiring them from this point forward.

If we don't, we may end up spending more time on the IP log during incubation.

[lossyrob]

@dr-jts what about us fools that don't use an IDE? :)

Having a no-unused import, no wildcard import rule could be fine, I'm just
worried about maintenance headaches. How would this be enforced? How do we
fix it when wildcard/unused slip through? I'd want to weigh that against
the burden of changing the js conversion mechanism to work with those types
of imports (I have no idea how much or little effort that would take).
On Jan 28, 2016 9:45 AM, "James Hughes" notifications@github.com wrote:

@dr-jts https://github.com/dr-jts If this is the kind of formatting the
Eclipse can automatically do, redoing it seems sensible. Signing a CLA
takes 5 minutes and covers the project forever, so I'd really suggest we
get in the habit of requiring them from this point forward.

If we don't, we may end up spending more time on the IP log during
incubation.

—
Reply to this email directly or view it on GitHub
https://github.com/dr-jts/jts/pull/13#issuecomment-176299772.

[dr-jts]

@lossyrob All good points, Rob.

The ultimate "enforcement" would the jsts conversion reporting any issues it found. Then someone would have to patch JTS to remove the wildcards. Hopefully this wouldn't happen too often.

But yes, even better would be to make the converter smarter! Perhaps only for common wildcards, like geom.* ?

[dr-jts]

Suggest splitting out the unused import commit - it can be merged as is.

[jnh5y]

In the event that we do wish to support non-wildcard imports, I'd suggest we commit to doing that for releases. For snapshots, perhaps @bjornharrtell could work off of a cleaned up fork.

Right before a release, we could run down a checklist and hit the optimize imports button in Eclipse...

[dr-jts]

@jnh5y Makes good sense to me.

[bjornharrtell]

Thanks for the input so far and congratulations on what seems like a very successful code sprint. I've already signed the CLA and the commits.

The auto translation effort is something I've worked on last six months as a potential successor the the current manual and outdated JavaScript port that I made in ca 2011. The work is tracked here bjornharrtell/jsts#178 and it's looking very promising.

This is a request to remove the wildcard imports as is to make it easier to translate the source to JavaScript.

On the Java side there are arguments for and against the practice of wildcard imports. The most common argument against it is that when using wildcard imports a new compilation unit can break an existing compilation unit, i.e by causing a name clash. This might or might not be a concern depending on the code base and extensibility but I suspect it's no real big issue for JTS.

For the translation to JavaScript the issue is that the concept of modules in modern JavaScript (aka EcmaScript 2015) is more restrictive than Java. Imports need to be explicit about which module that is imported. As far as I know the reason for this limitation is to make it easier to do static analysis of the dependencies between modules. It's not impossible to work around in translation but essentially I would have to resolve the wildcard imports in a preprocessing step similar to what Eclipse is doing. I've as of yet found no practical way of introducing that to my tool chain.

I'm totally ok with cleaning up this as needed, instead of enforcing per commit, before release or as need arises downstream.

[cuttlefish]

Hmm... yes, it seems as though Eclipse's organize imports is GUI only. Some quick Googling suggests that the alternatives are all tied to IDEs 😞

It'd be nice not to depend on JTS following a specific convention at the risk of breaking the autotranslator.

[bjornharrtell]

There are two sides to this. On one side without changes to the upstream I doubt I'll be able to translate without having to maintain a fork of JTS which partially defeats the purpose of translation. On the other side, I do not want to push for conventions or other changes that are undesirable upstream.

From what I've seen the Java community seem divided on the issue and in disagreement if wildcard imports makes for better or worse readability. I did find one more sound argument against wildcard imports which is that it increases compilation time. See http://www.javaperformancetuning.com/news/qotm031.shtml.

On the above page there is also readability argument that I agree with. "Finally, many people find that using imports with wildcards makes the source much less readable, since the reader also needs to figure out which package a particular class comes from, rather than just looking it up in the import statement.".

[lossyrob]

I think the age old argument of "which standard practice is better" is not the main point here, but that building a process on a hard-to-maintain standard, which we (well, Martin really) will have to try and manually enforce, is not the best way to have things not break downstream.

As far as downstream processes go, your saying your willing to clean this up when you need it downstream. This seems like it would be a manual process for you, and I'm wondering how it much more work it would be for you to run the eclipse import translation as part of the pre-processing step, like you say, with the potential of implementing it as an automated step in the future if someone finds a good way to do that. That's not really maintaining a fork, but just preprocessing the code before you auto generate. Is this something you're trying to build snapshots of as master of JTS gets merges? I can see how that would be annoying if there's a lot of snapshots to publish, it would be way better to have it completely automated, and that there's a legitimate desire to have the JTS codebase make a (potentially slightly healthier, as you argue) change to the style formatting requirements. I can also see how it would be annoying as a maintainer to agree to future back and forths with potential contributors to fix those styling issues, or to merge more PRs from an annoyed, hey-your-merge-broke-my-auto-convert when a wild card or unused import slips through. So, like you said, there's two sides to this. I just wish Eclipse had that functionality exposed via the CLI or some API!

[bjornharrtell]

I agree and is good reason to let this PR, or at least convention enforcement, wait until I've investigated a bit further if I can handle it in translation somehow.

That said, this PR could be viewed as simply a a one shot cleanup with a side effect that it is helpful for me until I can solve it in translation. I've other smaller but important changes to propose that I think will be needed to get a working translation done at all but this issue will in a way block them until I can solve it on my side.

[dr-jts]

@bjornharrtell The idea that for now this is a harmless one-shot cleanup is my thinking as well. It helps you out now, and it doesn't bother JTS.

Going forward we can look for better ways to address this, or simply deal with it manually however seems best.

Unless there are further objections, I'll accept the PR this weekend.

[lossyrob]

👍