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

Add Create Quick Filter action #1393

Merged

Conversation

SanjulaGanepola
Copy link
Contributor

Changes

Add Create Quick Filter action that allows users to quickly type a filter of the following format: LIB* or LIB/OBJ/MBR.MBRTYPE (OBJTYPE) where each parameter is optional except the library. For parameters which are not provided, the defaults will be used.

image

Checklist

  • have tested my change
  • updated relevant documentation
  • Remove any/all console.logs I added
  • eslint is not complaining
  • have added myself to the contributors' list in CONTRIBUTING.md
  • for feature PRs: PR only includes one feature enhancement.

@SanjulaGanepola
Copy link
Contributor Author

@liam I decided to go with the symbol-text icon as it looked like a better fit (let me know if you think list-filter might still be better). Also, let me know if you think the regular expression used for parsing can be improved.

@worksofliam
Copy link
Contributor

@SanjulaGanepola We are going to need a matching PR in the docs repo for this functionality too before this can be merged. Thanks!

https://github.com/halcyon-tech/docs/tree/main/docs

@SanjulaGanepola
Copy link
Contributor Author

Created a PR for the doc updates: codefori/docs#10

@worksofliam worksofliam requested a review from a team August 14, 2023 15:04
Copy link
Collaborator

@sebjulliand sebjulliand left a comment

Choose a reason for hiding this comment

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

Good job, it works as expected. I'd suggest a few changes to make the code flow unambiguous.

I also have two comments:

  • Is there an actual need to create a filter to list libraries?
  • There is a slight possibility that the new filter has the same name as an existing one. It may be better to name it using newFilter's content and check if this name is already taken (in which case you could append _1 to it).

Comment on lines 119 to 121
if(await vscode.window.showErrorMessage(`Error creating filter ${newFilter}`, `Retry`)){
vscode.commands.executeCommand(`code-for-ibmi.createQuickFilter`, node, newFilter);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the input is validated by the regex, is there any actual way this block could be reached?
If no, remove it and remove both node and newFilter as they serve no purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we are using validateInput with the regex already, this block should not be reachable so I removed it. Thanks for catching that

Comment on lines +125 to +127
config.objectFilters = objectFilters;
await ConnectionConfiguration.update(config);
this.refresh();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do this only if a new filter has been actually added (i.e. if newFilter is ok).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is already contained inside of the if(newFilter) so I think we know for sure the filter is added.

@SanjulaGanepola
Copy link
Contributor Author

  • Is there an actual need to create a filter to list libraries?

Although it may not be very common, I think it is more to support users who may want to browse for a library they would like to modify/delete/etc. Also in Project Explorer, we contribute several inline and right-click menu actions on libraries that include updating the library list, setting variables, or for migrating source to the local project.

image

  • There is a slight possibility that the new filter has the same name as an existing one. It may be better to name it using newFilter's content and check if this name is already taken (in which case you could append _1 to it).

The naming logic I used is the same as the default behaviour of the current Create Filter action (https://github.com/halcyon-tech/vscode-ibmi/blob/master/src/webviews/filters/index.js#L47) where the numbering is based on the current number of filters. Thus, I don't think it could be possible for the name to be an existing one? Could you provide a scenario in which it could happen?

Copy link
Collaborator

@sebjulliand sebjulliand left a comment

Choose a reason for hiding this comment

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

Changes look good, thanks! On second thought, the possibility that a "Filter 3" already exists when it gets created is so slim we won't worry about it for now.

@sebjulliand sebjulliand merged commit 8187776 into codefori:master Aug 19, 2023
1 check passed
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