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

[trace-debug] Marketplace release prep #20341

Merged
merged 4 commits into from
Nov 20, 2024
Merged

[trace-debug] Marketplace release prep #20341

merged 4 commits into from
Nov 20, 2024

Conversation

awelc
Copy link
Contributor

@awelc awelc commented Nov 20, 2024

Description

This PR contains changes required for the trace-debugger release in the Marketplace as an automatic dependency of the Move extension. We will publish the trace-debugger first and then update Move extension to pull it as its dependency.

Also added a new script to streamline Move extension publication to the VSCode marketplace

Copy link

vercel bot commented Nov 20, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sui-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 20, 2024 10:03pm
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Nov 20, 2024 10:03pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Nov 20, 2024 10:03pm
sui-typescript-docs ⬜️ Ignored (Inspect) Visit Preview Nov 20, 2024 10:03pm

@awelc awelc requested review from tedks and removed request for ronny-mysten November 20, 2024 01:45
@awelc awelc temporarily deployed to sui-typescript-aws-kms-test-env November 20, 2024 01:45 — with GitHub Actions Inactive
@awelc awelc self-assigned this Nov 20, 2024
Copy link
Contributor

@tzakian tzakian left a comment

Choose a reason for hiding this comment

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

Just the one sentence but otherwise LGTM

Comment on lines 111 to 113
This functionality is provided by automatically including Move Trace Debugging
[extension](https://marketplace.visualstudio.com/items?itemName=mysten.move-trace-debuggin)
where you can find more detailed information about trace-debugging and the current level of support.
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence is confusing to me, and I'm not sure it's saying this is automatically included in the larger Move IDE VSCode extension, or included in the move-trace-debugging extension (which is/is not automatically included with the IDE?).

I think rewording/clarifying this may be helpful :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link

@tedks tedks left a comment

Choose a reason for hiding this comment

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

LGTM modulo cleaning up the docs.

@@ -1,7 +1,9 @@
# Move

Provides language support for the Move programming language. For information about Move visit the
language [documentation](https://docs.sui.io/concepts/sui-move-concepts).
language [documentation](https://docs.sui.io/concepts/sui-move-concepts). It also provides early-stage
Copy link

Choose a reason for hiding this comment

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

But this isn't actually true, right? It lets you run the tracing command, it requires another extension to do trace debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For all intents and purposes it's true. When someone installs Move extension (after the next update of course), the Move Trace Debugging extension is installed automatically so from developer's perspective it's this extension that provides it (much like syntax highlighting is provided by a different extension and yet we list this feature in this README) The debugging instructions comment in the "Features" section clarifies this by saying that "This functionality is provided by automatically including Move Trace Debugging extension" (I did not think we need this particular piece of info right in at the top of this README page)

Copy link

Choose a reason for hiding this comment

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

Okay, could we say "The dependent extension, ${fully qualified extension name}, also provides..." just to be explicit?

Copy link
Contributor Author

@awelc awelc Nov 20, 2024

Choose a reason for hiding this comment

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

We do say that later in the README where we describe how to actually trace-debug Move code:

"If the opened Move source file is located within a buildable project, and you have the sui binary installed, you can trace-debug execution of Move unit tests within this project. This functionality is provided by this (Move) extension automatically including the Move Trace Debugging extension"

Not sure why we should throw this particular piece of information at the user right at the start, particularly that we don't at all mention that syntax highlighting is provided the same way (through a separate extension), unless you think we should do it as well. That being said, if you think it's crucial, I am not that strongly opposed to adding this info at the beginning, but still wanted to cast my vote here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going to land it as is but happy to change it (or stamp a change) if need be

@awelc awelc temporarily deployed to sui-typescript-aws-kms-test-env November 20, 2024 22:01 — with GitHub Actions Inactive
@awelc awelc merged commit bd683c7 into main Nov 20, 2024
54 checks passed
@awelc awelc deleted the aw/trace-debug-release branch November 20, 2024 22:37
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.

3 participants