-
Notifications
You must be signed in to change notification settings - Fork 93
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 ability to compare selected or open file with an IFS or local file path #2049
Conversation
Added ability to compare with Active File:
|
This reverts commit f6afeaa.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have left a quick review. Lack of member support is really the only major issue.
Also, can you please also add code-for-ibmi.compareCurrentFileWithMember
?
Added ability to compare with members (IBM i must be connected):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@julesyan Should we get this PR going? I have made some comments regarding the UX, which I hope you can fix. 🙏
src/instantiate.ts
Outdated
@@ -17,6 +17,7 @@ import { t } from './locale'; | |||
import { Action, BrowserItem, DeploymentMethod, MemberItem, OpenEditableOptions, WithPath } from "./typings"; | |||
import { ActionsUI } from './webviews/actions'; | |||
import { VariablesUI } from "./webviews/variables"; | |||
import { t } from './locale'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t
is already imported...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get errors when i remove it, where is it imported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@julesyan Only two small issues and then we're good to go! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@julesyan Well done - works as expected. 👍
Approved and merged.
@worksofliam I think this merged PR fixes issue #1902 right? |
Changes
Based on #1902
How to test this PR
Checklist
console.log
s I added