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.

Sign up for GitHub

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

Jump to bottom

Fix error opening a specific member reference in Go To File #1733

Merged
merged 9 commits into from
Dec 23, 2023
Merged

Fix error opening a specific member reference in Go To File #1733

merged 9 commits into from
Dec 23, 2023

Conversation

chrjorgensen
Copy link
Collaborator

@chrjorgensen chrjorgensen commented Dec 15, 2023 •
edited

Changes

This PR will fix issue #1732 by (re)adding the entered value to the list, thus allowing it to be selected (accepted).
This was a regression from the new member search in function Go To file.

Additional fixes are included:

  • Validation of entered member reference - previously the user could enter any extension for the member, and the member would still open. Now there is a warning if the member does not exist or the member type is wrong.
  • Search now works even if member and dot has been specified when entering '*'.
  • Member type is shown in uppercase like the other parts in a list item.
  • SQL statements and use of metadata columns are now consistent.
  • The Go To file dialog are kept open when the list is cleared.
  • Validation of entered streamfile before open - keeping the list available in case of error.

Checklist

  • have tested my change
  • Remove any/all console.logs I added
  • eslint is not complaining

@chrjorgensen chrjorgensen added the bug A confirmed issue when something isn't working as intended label Dec 15, 2023
@chrjorgensen chrjorgensen requested review from a team and removed request for worksofliam December 15, 2023 23:35
@worksofliam worksofliam linked an issue Dec 22, 2023 that may be closed by this pull request
Is the new Searching for members 🔍 feature replacing how Go to File... used to work? #1732
Closed
worksofliam
worksofliam requested changes Dec 22, 2023
View reviewed changes
Copy link
Contributor

@worksofliam worksofliam left a comment

Choose a reason for hiding this comment

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

Left one small suggestion. Code looks pretty good otherwise!

src/instantiate.ts Outdated Show resolved Hide resolved
worksofliam
worksofliam requested changes Dec 23, 2023
View reviewed changes
Copy link
Contributor

@worksofliam worksofliam left a comment

Choose a reason for hiding this comment

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

Only one real issue with casing, other than that it works as expected now!

src/instantiate.ts Outdated
vscode.window.showInformationMessage(`Cleared list.`);
quickPick.hide()
} else {
const selectionSplit = selection.split('/')
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like selectionSplit doesn't account for case, so if a lowercase value is provided, the rest of this code fails because it expects members in uppercase for the SQL statement.

Suggested change
const selectionSplit = selection.split('/')
const selectionSplit = selection.toUpperCase().split('/');

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The selection will always be in uppercase, if it's a member:

  • if the member is entered by the user, the value is put in the list in uppercase
  • if the member is from a search, it is in uppercase by SQL
    So it should not be necessary to convert to uppercase. We also don't want to convert streamfiles to uppercase.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you confirm this is user error then? I have all these lowercase listings in my quick pick from prior usage.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

I also have this enabled, so it looks like it is adding lowercase names to the list!
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it's this line of code storing the values, including as lowercase when that option is enabled: https://github.com/chrjorgensen/code-for-ibmi/blob/d863157505d93b76388c9c9092fee9c398375394/src/views/objectBrowser.ts#L216

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The uppercase issue has been fixed.

src/instantiate.ts Show resolved Hide resolved
worksofliam
worksofliam approved these changes Dec 23, 2023
View reviewed changes
Copy link
Contributor

@worksofliam worksofliam left a comment

Choose a reason for hiding this comment

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

  • My lowercase cache works now
  • Search/filter still working as expected

Beautiful work @chrjorgensen!

@worksofliam worksofliam merged commit f94a77e into codefori:master Dec 23, 2023
1 check passed
@chrjorgensen chrjorgensen deleted the fix/issue-1732 branch December 23, 2023 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@worksofliam worksofliam worksofliam approved these changes

Assignees
No one assigned
Labels
bug A confirmed issue when something isn't working as intended
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Is the new Searching for members 🔍 feature replacing how Go to File... used to work?
2 participants
@chrjorgensen @worksofliam