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

Sort maven project list in Explorer and hide invalid pom.xml files #118

Merged
merged 4 commits into from
Sep 3, 2018
Merged

Sort maven project list in Explorer and hide invalid pom.xml files #118

merged 4 commits into from
Sep 3, 2018

Conversation

owenconti
Copy link
Contributor

  • Fixed a TS typing error on a boolean variable.
  • Updated _ensureMavenProjectParsed of MavenProjectNode to become public.
  • Alphabetically sort the array of MavenProjectNode items in WorkspaceFolderNode.

screen shot 2018-08-31 at 6 21 31 pm

If I've made any mistakes or if there is a better way of approaching the problem, please let me know.

@msftclas
Copy link

msftclas commented Sep 1, 2018

CLA assistant check
All CLA requirements met.

Copy link
Member

@Eskibear Eskibear left a comment

Choose a reason for hiding this comment

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

In reviewing this PR, I find a issue that originally exists. When you have more than one pom.xml files in the workspace, if for some reason one of the pom.xml file cannot be successfully parsed, then all the other projects cannot be rendered in the explorer. (because rejected promise is not properly handled.)

Potential promise rejects in _sortChildren. It can fail the getChildren. This PR is facing the same issue.

@@ -53,7 +53,7 @@ export class MavenProjectNode extends NodeBase {
return this._mavenProject.modules.length > 0;
}

private async _ensureMavenProjectParsed(): Promise<void> {
public async ensureMavenProjectParsed(): Promise<void> {
if (!this._mavenProject) {
const pom: {} = await Utils.parseXmlFile(this._pomPath);
Copy link
Member

Choose a reason for hiding this comment

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

Notice here it parses the pom.xml file, error would possibly be thrown.


private async _sortChildren(): Promise<void> {
for ( const projectNode of this._children ) {
await projectNode.ensureMavenProjectParsed();
Copy link
Member

Choose a reason for hiding this comment

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

Here for each pom.xml, you parse the content here. And if error occurs when parsing any of the pom.xml files, the it rejects.

src/Utils.ts Show resolved Hide resolved
@owenconti
Copy link
Contributor Author

Hi @Eskibear,

I've updated the code to handle errors thrown in the ensureMavenProjectParsed function. I also updated getChildren in WorkspaceFolderNode to make sure the MavenProjectNode is valid before considering it a children.

Here is a screenshot showing the difference:

screen shot 2018-09-01 at 4 37 06 pm

Valid pom.xml ^

screen shot 2018-09-01 at 4 37 16 pm

Invalid pom.xml ^ (notice the project missing from the Explorer list)

Copy link
Member

@Eskibear Eskibear left a comment

Choose a reason for hiding this comment

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

Up to this commit, I see you choose to directly remove the item containing invalid pom.xml (as mentioned in #119 (comment) ). That's ok for me since it's better than before anyway. We can discuss later in that issue about whether to simply remove or show the invalid pom entry for users to fast navigate.

Overall it looks good to me, and there are just some nits to be resolved before we can merge it. Please take a look.

src/explorer/model/MavenProjectNode.ts Outdated Show resolved Hide resolved
src/explorer/model/MavenProjectNode.ts Outdated Show resolved Hide resolved
src/explorer/model/MavenProjectNode.ts Outdated Show resolved Hide resolved
src/explorer/model/WorkspaceFolderNode.ts Outdated Show resolved Hide resolved
src/explorer/model/WorkspaceFolderNode.ts Outdated Show resolved Hide resolved
@owenconti owenconti changed the title Sort maven project list in Explorer Sort maven project list in Explorer and hide invalid pom.xml files Sep 2, 2018
…e accruately describle it does.

Remove unused `async` on WorkspaceFolderNode._sortChildren.
@owenconti
Copy link
Contributor Author

I think I got all the changes. I also changed _ensureMavenProjectParsed to _parseMavenProject to more accurately describe what it does.

Apologies, I hadn't seen that you made an issue for the parsing of invalid pom.xml files.

Copy link
Member

@Eskibear Eskibear left a comment

Choose a reason for hiding this comment

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

LGTM!

@Eskibear Eskibear merged commit e6f961c into microsoft:master Sep 3, 2018
@Eskibear Eskibear mentioned this pull request Sep 3, 2018
5 tasks
@Eskibear Eskibear added this to the 0.11.0 milestone Sep 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants