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.

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

Remove import redundancy; Fix project setting UI #246

Merged
merged 17 commits into from
Jan 19, 2017

Conversation

wisechengyi
Copy link
Collaborator

@wisechengyi wisechengyi commented Jan 14, 2017

Problem

Once the project is created, under Settings -> Build Tools -> Pants, Linked Pants project settings are not working. Also 3 pants calls were made in order to determine the targets under project path, which is 2 more than necessary.

E.g. Imported targets are now marked incorrectly, and any attempt to change the setting will be ignored.
screen shot 2017-01-18 at 9 56 23 am

Solution

screen shot 2017-01-18 at 10 31 26 am

Other change:

  • Move Use IDEA project JDK for Pants compilation setting into Project setting. Earlier it is in a separate form, which causes confusion.
  • Remove unused "load depedees" option.

@wisechengyi wisechengyi changed the title Remove import redundancy Remove import redundancy; Fix project setting Jan 18, 2017
Copy link
Contributor

@baroquebobcat baroquebobcat left a comment

Choose a reason for hiding this comment

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

Nice clean up. I've got a few comments.

I think the summary ought to also note that the dependencies checkbox was removed.

}

public void onProjectPathChanged(@NotNull final String projectPath) {
myTargetSpecs.clear();
if (lastPath.equals(projectPath)) {
lastPath = projectPath;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why assign here if they are equal? This could be reduced to being just a guard clause.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oops. corrected. thanks!

content.add(pane, lineConstraints);
}

// It is silly `CheckBoxList` does not provide an iterator.
Copy link
Contributor

Choose a reason for hiding this comment

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

You could add a static external iterator method that accepts a java.util.function.Consumer. Then you could simplify this and the other loop in applyExtraSettings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call! Fixed

@@ -18,7 +18,7 @@ public void testProjectName() throws Throwable {

PantsExecutionSettings settings = new PantsExecutionSettings(
Collections.singletonList(deepDir),
false, // with dependees. does not matter here
// with dependees. does not matter here
Copy link
Contributor

Choose a reason for hiding this comment

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

xx

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

corrected.

import java.io.File;

public class PantsProjectSettingsTest extends OSSPantsIntegrationTest {
public void testDirectory() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This first test is a little hard to follow.

How would you feel about breaking out some utility methods for the common components between it and testBuildFile. I'd also like to see some tests for the other states the control can be in, ie missing a file or non-pants project file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added some negative test cases, separate and doc'ed util methods.

Copy link
Contributor

@peiyuwang peiyuwang left a comment

Choose a reason for hiding this comment

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

only minor comments

private final boolean myLibsWithSourcesAndDocs;
private final boolean myUseIdeaProjectJdk;
private final boolean myEnableIncrementalImport;
private final List<String> myTargetSpecs;

private static final List<String> DEFAULT_TARGET_SPECS = Collections.emptyList();
private static final boolean DEFAULT_WITH_DEPENDEES = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

WITH_DEPENDEES, then using WITH_DEPENDEES and !WITH_DEPENDEES is more readable then true or false.

ditto other similar places

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

corrected.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

DEFAULT_WITH_DEPENDEES itself is removed because it is never used, but the readability is applied elsewhere.

@@ -113,7 +113,7 @@ private void convertToPantsProject() {
*/
final List<String> targetSpecs = PantsUtil.gson.fromJson(serializedTargets, PantsUtil.TYPE_LIST_STRING);
final PantsProjectSettings pantsProjectSettings =
new PantsProjectSettings(targetSpecs, projectPath, false, true, false);
new PantsProjectSettings(targetSpecs, projectPath, true, false, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

see the other readability comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

corrected.

@wisechengyi
Copy link
Collaborator Author

wisechengyi commented Jan 19, 2017

Added Remove unused "load depedees" option. under description. The function was implemented, but the GUI never provided a way to configure it, hence removed in this review.

Copy link
Contributor

@baroquebobcat baroquebobcat left a comment

Choose a reason for hiding this comment

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

lgtm

@wisechengyi wisechengyi changed the title Remove import redundancy; Fix project setting Remove import redundancy; Fix project setting UI Jan 19, 2017
@wisechengyi wisechengyi merged commit adbfaf1 into pantsbuild:master Jan 19, 2017
@wisechengyi
Copy link
Collaborator Author

Thanks gents!

@wisechengyi wisechengyi mentioned this pull request Mar 2, 2017
wisechengyi added a commit that referenced this pull request Mar 2, 2017
# Changes notes

## Bug fixes
* Remove import redundancy; Fix project setting UI (#246) 
* Fix language level not set (#249)
* Fix JDK not set correctly from idea-plugin import (#267)

## Performance
* Skip looking for JDK when there is one set already (#261)
* Event based project view focusing (#263) 

## Usability
* Add ability to cancel Pants process (#258) 

## Metrics
* Capture indexing duration on the fly (#265) 
* Log noop PantsCompile and duration between PantsCompile invocation and last file edit (#241)
@wisechengyi wisechengyi deleted the no_compiler_form branch July 31, 2017 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants