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

feat(VSCode): Define metadata API #1435

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

lordrip
Copy link
Member

@lordrip lordrip commented Sep 13, 2024

No description provided.

Copy link

codecov bot commented Sep 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@c3cf5e0). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1435   +/-   ##
=======================================
  Coverage        ?   67.53%           
  Complexity      ?       25           
=======================================
  Files           ?      266           
  Lines           ?     7601           
  Branches        ?     1520           
=======================================
  Hits            ?     5133           
  Misses          ?     2465           
  Partials        ?        3           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


/**
* Retrieve resource content
* @param path The path of the resource, relative to the file being edited
Copy link
Member

Choose a reason for hiding this comment

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

just a note to ensure that you have in mind that this info will need to be computed on your side.
In maven based project, I think the path which is mentioned in the camel route is the one from the classpath, so from src/main/resources usually.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we would need to use a File picker of some sorts, the expectation would be to intercept that filepath and make it relative to the open file

Copy link
Member

Choose a reason for hiding this comment

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

This could work only on first edit.
When reopening the editor, it won't go through the File picker.

Copy link
Member Author

@lordrip lordrip Sep 13, 2024

Choose a reason for hiding this comment

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

no, it won't, but the path will be already stored if needed, so no problem for the next time

Copy link
Member

Choose a reason for hiding this comment

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

when moving from standalone to a Maven projects, need to check where the xslt files are moved and if the saved path needs to be updated then

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this conversation will need to happen once we get more information about the topic, at the moment of the implementation. I'll remove the comment relative to the file being edited so we can move forward.

@lordrip lordrip force-pushed the feat/provide-metadata-api branch from 4a94a79 to baaa4da Compare September 13, 2024 12:46
@lordrip lordrip force-pushed the feat/provide-metadata-api branch from baaa4da to bd46bba Compare September 16, 2024 07:39
@lordrip lordrip requested a review from apupier September 16, 2024 07:39
Copy link
Member

@apupier apupier left a comment

Choose a reason for hiding this comment

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

either need to remove the path parameter of the currently edited file from setMetadata and getMetadata, or to add it also in getResourceContent to make it coherent of either rely on the parameters for the currently edited file or rely on the channelAPI attributes.

I think we can rely on the channel api and remove the path parameter of getMetadata and setMetadata

* @param path The path of the resource, relative to the file being edited
* @param content The content to be saved
*/
saveResourceContent(path: string, content: string): Promise<void>;
Copy link
Member

Choose a reason for hiding this comment

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

Should we declare the exception in case there is an exception while trying to save the file?

Copy link
Member Author

Choose a reason for hiding this comment

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

It might be a good idea for the future, at this moment, I think whenever an exception happens, the notification and handling should be delegated to the host. For VSCode, this is what I think should happen:

  1. A file is being saved
  2. An exception is thrown from the fs or the MA module that controls the files being written
  3. VSCode captures this exception and notifies the user through the VSCode alert functionality
  4. We could choose to rethrow the exception to the Editor as well, just in case we want to either handle the UI part differently or notify the user as well.

@lordrip lordrip force-pushed the feat/provide-metadata-api branch from bd46bba to 68a433f Compare September 16, 2024 08:10
@lordrip lordrip requested a review from apupier September 16, 2024 08:11
@lordrip lordrip force-pushed the feat/provide-metadata-api branch from 68a433f to 12a042a Compare September 16, 2024 09:46
Copy link

sonarcloud bot commented Sep 16, 2024

@lordrip
Copy link
Member Author

lordrip commented Sep 16, 2024

Merging this as a first iteration of the needed API, will be revisited once the implementation is more mature.

@lordrip lordrip merged commit 972d250 into KaotoIO:main Sep 16, 2024
12 checks passed
@lordrip lordrip deleted the feat/provide-metadata-api branch September 16, 2024 13:32
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