Data editor: add profile source target selection#1564
Data editor: add profile source target selection#1564Ontiomacer wants to merge 2 commits intoapache:mainfrom
Conversation
|
Firstly, could you update the PR description w/ documentation updates, if any. This is required and runs against a CI job to verify. Take a look at some other PRs and the PR template for how it is formatted w/ markdown. Secondly, the TS/Svelte formatting needs to be resolved. There is a script in the I will test out the functionality for this PR when I get a chance sometime today. |
|
Thanks for the feedback. I’ve updated the PR description to explicitly cover documentation requirements per the template. I’m also running the project’s formatting script and will amend the commit if it introduces any changes. I did not attempt to resolve unrelated pre-existing Svelte check warnings outside the scope of this PR. Let me know if you’d like me to address any specific formatting issues beyond that. |
|
Hey @stricklandrbls, You’d mentioned earlier that you were also working around this area and that we could tackle it together, so I wanted to see if you’ve had a chance to try out the changes on your end. From my side, the target selection behaves as expected, but I’d really value your take on whether the approach and wiring look correct or if anything should be adjusted. Also, if there are other issues in the daffodil-vscode repo where help would be useful right now, feel free to point me in that direction — happy to pick up more work. Thanks, and no rush at all. Just wanted to make sure I’m aligned and well updated |
|
Also lastly @stricklandrbls i hope the changes you requested me to do are aligned properly in my pr description or is there something left ?? |
|
You're adding a feature that requires a user to make a selection (where they previously did not), but state that no documentation needs to be updated? |
|
@Ontiomacer, would you like me to assist you with passing the lint tests? |
|
Hey collaborators @rthomas320 @lrbarber @JeremyYao I am working on this and I'll update this pr soon |
|
Hey @JeremyYao Locally: yarn prettier src -c → passes yarn prettier . -c → passes yarn svelte-check → fails This looks like a dependency/toolchain mismatch between svelte, svelte-check, and the TS DOM lib used by CI. Happy to help fix it, but it seems orthogonal to the feature changes here. Please advise how you’d like me to proceed. |
|
@lrbarber If there’s a specific wiki page or format you’d like me to follow, please let me know — otherwise I’ll mirror the existing style and keep the update minimal and focused. |
|
@JeremyYao |
|
@Ontiomacer All you should need to do is run the below command locally: yarn lint:fixThis should auto format any files that are causing lint issues. After that you should just need to commit the updated files. |
Signed-off-by: Tejas Tiwari <tt160705@outlook.com>
Signed-off-by: Tejas Tiwari <tt160705@outlook.com>
e988f7a to
b88ef20
Compare
|
I force-pushed to clean up the branch history and retrigger CI. |
Approved workflows to run |
|
Ok @JeremyYao i see the sqash commit error and a documentation error to ci I will fix the squash commit error shortly what should I do for documentation error? |
The documentation error checks to see if the checkbox for determining if documentation for the PR should be added. Given that this is an enhancement (addition to the extension), I suggest re-adding the |
|
@Ontiomacer - I apologize for the radio silence recently, I have been tied up in a ton of other things plus the holidays. I'm going to checkout this branch and look at the functionality. If I see any issues I'll bring them up. If not then once the CI errors are fixed I will approve. |
| function requestSessionProfile(startOffset: number, length: number) { | ||
| lastRequestedTarget = profileTarget | ||
| setStatusMessage( | ||
| `Profiling bytes from ${startOffset} to ${startOffset + length}...`, |
There was a problem hiding this comment.
I would remove the ... tail of the message. The status message indicates as if the profiler is currently loading something.
| }) | ||
| } | ||
|
|
||
| let lastProfileTarget: 'editor' | 'disk' | null = null |
There was a problem hiding this comment.
Given that this variable instance type is used in multiple places it should be extracted to a concrete type definition:
type ProfileTarget = 'editor' | 'disk'Then change the appropriate variable's types:
let lastProfileTarget: ProfileTarget | undefined = undefined
let lastRequestedTarget: ProfileTarget = 'editor'| ) | ||
| vscode.postMessage({ | ||
| command: MessageCommand.profile, | ||
| data: { |
There was a problem hiding this comment.
Create a data object above this postMessage call and deconstruct it into the data property:
type ProfileMsg = {
startOffset: number,
length: number,
target: ProfileTarget
}
...
const profileTargetData: ProfileMsg = {
startOffset,
length,
target: profileTarget === 'disk' ? 'disk' : 'editor'
}
vscode.postMessage({
...
data: {...profileTarget}
}I'm also unsure as to why the target would be set to undefined if the target was 'editor' since there's only 2 requestable types.
|
|
||
| try { | ||
| if (target === 'disk') { | ||
| const tempSession = await createSession( |
There was a problem hiding this comment.
Calling the createSession API function is emitting the following error from Omega Edit server:
rejected promise not handled within 1 second: createSession error: 13 INTERNAL: Failed to create session: ALREADY_EXISTS
I don't currently have a resolution off the top of my head so you may have to read up on the exposed API functions to work around this.



Closes #1563
Description
This PR adds support for profiling different data targets in the Data Editor, allowing users to choose between profiling:
This resolves cases where profiling previously ignored unapplied edits and aligns the profiler behavior with user expectations.
Wiki
Review Instructions
Confirmation Testing
Regression Testing