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

Refactor and reorganize code structure #2455

Merged
merged 47 commits into from
Jan 21, 2025
Merged

Conversation

worksofliam
Copy link
Contributor

@worksofliam worksofliam commented Jan 14, 2025

A large PR to separate our IBM i API from the VS Code namespace (vscode) entirely.

  • Everything in ./src/api no longer imports vscode or anything in directories above it (other than npm modules of course). In theory this means the API directory is completely cut off from the extension, and is imported like a unique module.
  • Components didn't depend on VS Code either, so they have been moved into the API directory.
  • The root typings.ts file has been split up due to the fact that typings.ts imported vscode. Instead, the types file has been split up, but the typings.ts correctly exports all types as it did before.
  • The IBMi class has had a lot of parameter changes. We now make use of callbacks where we previously had shown messages to the user. This did make the IBMi#connect code smaller since we moved most of the UI logic to another file.
  • src/api/Tools.ts file had to be split up since it included vscode too.
  • There is a new ui directory, which contains the views directory, as well as a bunch of other code specific to VS Code. For example, diagnostics are handles here, as well as Actions. Actions, both the UI and execution, used to be handled in src/api/CompileTools.ts, but had to be split up due to the mix of UI and backend execution code.
  • Configuration: wow, this one was complex. I am not sure even sure if I am happy with the final result, but I am pleased with the outcome. Previously, our IBM i APIs were mixed in with the vscode API - whoops! I had to virtualize all the config classes and then extend them to new classes which uses the VS Code API.
    • We have two concepts of configuration in our extension: storage and config. Both of these needed virtualizing.
    • I added a readme file in the code to explain the differences of these to the users.
    • The IBMi#connect function actually calls into the connection config to load in config for a connection. This logic stayed the same, though I am not the biggest fan. It'd be nice if this happened outside of this class.
  • I have added vitest and have it connecting to an IBM i based on environment variables. This means we will be able to move away from our custom test framework for our API, but keep it for UI tests. This means we can automate our tests finally. There will be a follow up PR (Swap some test cases to vitest #2462) for the test case work.

How to test

  • Run the test cases and ensure they're passing (except that bloody tab test that has been failing for years)
  • Use the extension and check nothing is broken. Example:
    • Change current library
    • Run an action
    • Search the IFS for a string, or a source file

Todo

  • Run the existing test cases to ensure everything is still passing
  • Use the extension to ensure the UX has not changed

… imports from tools module

Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
@worksofliam worksofliam marked this pull request as ready for review January 15, 2025 16:19
@worksofliam worksofliam mentioned this pull request Jan 16, 2025
3 tasks
Copy link
Member

@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.

Commented some initial findings in my testing and review of the code. Will comment more as I continue testing

Noticed there are some APIs (specifically used by PE which have been moved out). Not a major issue since I can just copy over the functions to PE. Just wanted to make sure this was intentional.

  • IBMiContent
    • ifsFileToToolTip
    • objectToToolTip
    • memberToToolTip
    • sourcePhysicalFileToToolTip
  • Tools:
    • getGitAPI

@worksofliam
Copy link
Contributor Author

@SanjulaGanepola yes. Perhaps we could still expose these APIs, since they have only moved.

I can take a look at this.

@worksofliam
Copy link
Contributor Author

worksofliam commented Jan 17, 2025

@SanjulaGanepola I kind of half-solved the tools export issue. I can't merge the two back due to the use of vscode in one of them, but I do now export both: tools and vscodeTools - vscodeTools has the git API and the tooltip functions.

The tools export now works as it did before. No breaking change there.

I also solved the other problems like normalising the Windows path in the API instead of letting the extensions do it and also added the channel logic back.

Please review again. Cheers!

@SanjulaGanepola
Copy link
Member

@worksofliam The prepublish script command in types/package.json needs to be taken a look at as it is currently removing some folders containing types which were moved out of the api folder such as CustomUI and DeployTools.

@worksofliam
Copy link
Contributor Author

@SanjulaGanepola For later: I have fixed the types issue. We'll check it out together.

Copy link
Member

@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.

Awesome work. Love this!

@worksofliam worksofliam merged commit 4dc4eea into master Jan 21, 2025
3 checks 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.

2 participants