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

Move Pants output to console view instead of notification channel #242

Merged
merged 27 commits into from
Jan 13, 2017

Conversation

wisechengyi
Copy link
Collaborator

@wisechengyi wisechengyi commented Jan 10, 2017

Problem

Pants output used to go out via the IntelliJ notification channel with a few down sides:

  1. No auto scrolling supported. If the message gets long, user has to manually scroll the message window.
  2. Output does not show up exactly like the command line. E.g. compile error indicator ^ will show up off.
  3. Cannot take advantage of IntelliJ hyperlink for user to click on the error to directly dump to what the error stems.

screen shot 2017-01-11 at 11 13 21 am

Solution

Use a text console view, which has

  1. Auto scroll by default
  2. Correct output format
  3. Capability to inject hyperlinks

screen shot 2017-01-11 at 11 18 58 am

@wisechengyi wisechengyi changed the title Move Pants output goes to console instead of notification channel Move Pants output to console view instead of notification channel Jan 10, 2017
@wisechengyi wisechengyi requested review from baroquebobcat, benjyw, areitz and peiyuwang and removed request for areitz January 11, 2017 19:21
@wisechengyi wisechengyi requested a review from stuhood January 11, 2017 19:27
}


public void testErrorMessageWithLocation2() {
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the difference from the java test case? seems redundant

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! I will remove having certain file extensions as a requirement, because eventually we will check if it is a valid file path when constructing the hyperlink.


try {
// filePath path is between tag and first colon
String filePath = splitByColon[0].substring(splitByColon[0].indexOf(tag) + tag.length()).trim();
Copy link
Contributor

Choose a reason for hiding this comment

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

some negative examples from old log

[error] Compile failed at Dec 9, 2016 6:33:30 AM [0.080s]
[error]         Assert.assertEquals("0:00:00.000", ManagementUtils.getAverageAge(d0));

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's okay for String filePath to be Assert.assertEquals("0 for example, because the hyperlink construction will check if it is an actual file. If not, it will there will be no hyperlinks.

I will add more doc to this to be more clear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

More doc added, as well as test case to cover this.

OpenFileHyperlinkInfo linkInfo = new OpenFileHyperlinkInfo(
project,
result.get().getFile(),
result.get().lineNumber - 1, // line number needs to be 0 indexed
Copy link
Contributor

Choose a reason for hiding this comment

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

getLineNumber getColumnNumber

Copy link
Contributor

Choose a reason for hiding this comment

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

q: in case we have a invalid line number either <0 or > max actual line# what will happen? as long as it doesn't crash.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. corrected.
  2. Manually tested by intentionally feeding it with line number -1, there is no crash, just the cursor is somewhere random.

Copy link
Collaborator Author

@wisechengyi wisechengyi Jan 13, 2017

Choose a reason for hiding this comment

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

  1. Non numbers will be caught by NumberFormatException, and return no link.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@wisechengyi wisechengyi merged commit 9cce5de into pantsbuild:master Jan 13, 2017
@wisechengyi
Copy link
Collaborator Author

Thanks @peiyuwang !

@wisechengyi wisechengyi mentioned this pull request Jan 14, 2017
wisechengyi added a commit that referenced this pull request Jan 15, 2017
* Add usage logging extension point (#239)
* GUI import fix (#244)
* Move Pants output to console view instead of notification channel (#242)
@wisechengyi wisechengyi deleted the console 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.

2 participants