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

Open path from URI #2038

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Open path from URI #2038

wants to merge 8 commits into from

Conversation

worksofliam
Copy link
Contributor

@worksofliam worksofliam commented May 7, 2024

Changes

  • Add new url for opening files at a specific path
  • Add improved error handling in the new URI
  • When the host is provided, but the user is not connected, then automatically connect if the connection is defined
  • Nice to have: make sure Sandbox mode still works

How to test this PR

  • vscode://halcyontechltd.code-for-ibmi/open?path=/home/liama/xyz
  • vscode://halcyontechltd.code-for-ibmi/open?host=1.1.1.1&path=/home/liama/xyz

Examples:

  1. Run the test cases
  2. Expand view A and right click on the node
  3. Run 'Execute Thing' from the command palette

Checklist

  • have tested my change
  • have created one or more test cases
  • updated relevant documentation
  • Remove any/all console.logs I added
  • have added myself to the contributors' list in CONTRIBUTING.md

@worksofliam worksofliam marked this pull request as draft May 7, 2024 14:58
@worksofliam worksofliam changed the title Feature/uri_cleanup Open path from URI May 7, 2024
@worksofliam worksofliam added this to the 3.0.0 milestone May 7, 2024
@worksofliam worksofliam linked an issue May 7, 2024 that may be closed by this pull request
@worksofliam worksofliam self-assigned this May 8, 2024
@worksofliam worksofliam marked this pull request as ready for review June 3, 2024 07:42
@worksofliam worksofliam added the build Build will be available inside PR. label Jun 3, 2024
Signed-off-by: worksofliam <[email protected]>
Copy link
Contributor

github-actions bot commented Jun 3, 2024

👋 A new build is available for this PR based on 7a6a00b.

@julesyan
Copy link
Collaborator

julesyan commented Jun 7, 2024

If they have no current connection would it beneficial to allow the user to pick one? Instead of a warning message.

Also I am unsure the purpose of the host param as the below two scenarios arent covered:

  • Scenario 1: if you arent connected it doesnt auto connect to the given host, isntead it just gives a warning it cannot open the file. I think it should ask to auto connect if the given host is on the list. If the host is listed more than once then give me the option to pick which connection to initiate
  • Scenario 2: if you are connected to the wrong host, it just prompts to open with the current connection. We should also have the option to switch to the correct connection, again if there is more that one listed conneciton to the same host then allow me to pick which connection to initiate

@worksofliam
Copy link
Contributor Author

@julesyan Thanks for your great testing.

  1. Yes, we should automatically connect to the host given it is in the list.
  2. Right now we don't even have the internal logic to disconnect and reconnect to a new system automatically. Let me see if this is even possible (since we do have APIs to do both separately.)

@worksofliam worksofliam marked this pull request as draft July 1, 2024 16:56
@worksofliam
Copy link
Contributor Author

@julesyan This works now:

vscode://halcyontechltd.code-for-ibmi/open?host=oss74dev&path=/home/LIAMA/hebrew.txt

@julesyan
Copy link
Collaborator

Tested locally and found no more issues, looks good!

@worksofliam worksofliam marked this pull request as ready for review September 11, 2024 16:18
Copy link
Contributor

@SanjulaGanepola SanjulaGanepola left a comment

Choose a reason for hiding this comment

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

@worksofliam I did some testing with this change and noticed a few issues. Refer to my comments below.

window.showWarningMessage(t(`uriOpen.missingPath`));
}
} else {
window.showWarningMessage(t(`uriOpen.noConnection`));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
window.showWarningMessage(t(`uriOpen.noConnection`));
window.showWarningMessage(t(`uriOpen.missingPath`));

commands.executeCommand(`code-for-ibmi.openEditable`, path);
}
} else {
window.showWarningMessage(t(`uriOpen.missingPath`));
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the example you gave in this PR:

vscode://halcyontechltd.code-for-ibmi/open?path=/home/liama/xyz

I assume this means that the file will be opened based on the active connection. If so, is this else block a mistake because this will not open any file and just give this warning even though I am connected. In the case there is no connection, shouldn't the message be:

window.showWarningMessage(t(`uriOpen.noConnection`));

@worksofliam worksofliam marked this pull request as draft November 1, 2024 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Build will be available inside PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New URI handler to open files
3 participants