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

Update: list all existing pligins when checking plugin list, not just installed #3080

Merged
merged 10 commits into from
Jan 13, 2024

Conversation

Jim8y
Copy link
Contributor

@Jim8y Jim8y commented Jan 10, 2024

This pr update the plugins command.

In the past, this command only list the local installed plugins, now it also list other plugins avaliable for neo-cli.

The reason for adding this pr is because the plugins name is hard to remember, yet checking them have to go to the github release log.

Uploading Screenshot 2024-01-10 at 8.04.10 PM.png…

@Jim8y
Copy link
Contributor Author

Jim8y commented Jan 10, 2024

Screenshot 2024-01-10 at 8 04 59 PM

@cschuchardt88
Copy link
Member

cschuchardt88 commented Jan 10, 2024

Can you fix the alignment (padding)?

@Jim8y
Copy link
Contributor Author

Jim8y commented Jan 10, 2024

Can you fix the alignment (padding)?

what format you think is better?

@shargon
Copy link
Member

shargon commented Jan 10, 2024

I like the idea :)

@cschuchardt88
Copy link
Member

Move over the names so they are aligned with each other and maybe put not installed and have it grayed out text. Also add the plugin descriptions if possible.

image

@Jim8y
Copy link
Contributor Author

Jim8y commented Jan 11, 2024

Screenshot 2024-01-11 at 9 53 27 AM

@Jim8y
Copy link
Contributor Author

Jim8y commented Jan 11, 2024

But we can't get description for uninstalled, its contained within the plugins.

@@ -242,12 +242,16 @@ private async void OnPluginsCommandAsync()
if (installedPlugin.Length == 1)
{
var plugin = $"(installed) {p}";
plugin = plugin.PadLeft(25);
Copy link
Member

@cschuchardt88 cschuchardt88 Jan 11, 2024

Choose a reason for hiding this comment

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

You can do something like this for padding in dotnet like it's easier and cleaner. $"(Installed) {p, 25}"

@shargon
Copy link
Member

shargon commented Jan 11, 2024

But we can't get description for uninstalled, its contained within the plugins.

Then it's better like this:

Installed:

  • xxxx
  • -xxxx
    -Availables:
  • xxxx, yyyy, zzzz,...

@shargon
Copy link
Member

shargon commented Jan 11, 2024

label string
An alternate short description of the asset. Used in place of the filename.

We are not filling this field, but we can add a short description to the asset

@cschuchardt88
Copy link
Member

cschuchardt88 commented Jan 11, 2024

label string
An alternate short description of the asset. Used in place of the filename.

We are not filling this field, but we can add a short description to the asset

@Jim8y We can add a json manifest file on github to download? With plugin information in it.

@Jim8y
Copy link
Contributor Author

Jim8y commented Jan 11, 2024

Another thing.

@cschuchardt88
Copy link
Member

@Jim8y if you need help, I can help

@Jim8y
Copy link
Contributor Author

Jim8y commented Jan 11, 2024

@Jim8y if you need help, I can help

It not about that, just not a thing this pr tries to work. Manifest maybe, not this pr.

@shargon
Copy link
Member

shargon commented Jan 11, 2024

label string
An alternate short description of the asset. Used in place of the filename.

We are not filling this field, but we can add a short description to the asset

@Jim8y We can add a json manifest file on github to download? With plugin information in it.

It's a good idea

@Jim8y
Copy link
Contributor Author

Jim8y commented Jan 11, 2024

Manifest is a manifest of plugins, which is not in this repo yet. Good idea or not. It's another thing. And I don't think downloading a file directly from the source is a good thing.

@cschuchardt88
Copy link
Member

cschuchardt88 commented Jan 11, 2024

Manifest is a manifest of plugins, which is not in this repo yet. Good idea or not. It's another thing. And I don't think downloading a file directly from the source is a good thing.

It would be published to the Release? We download plugins from github. Why not? Or even better. have manifest with-in the neo-cli directory. Same as config.json or add it to config.json. Also have download link Url be configurable. So we can target PRs

@Jim8y
Copy link
Contributor Author

Jim8y commented Jan 11, 2024

I don't want discuss it in this pr. This pr only focus on solve a simple issue which is the name of all plugins. Yes, we may get description from somewhere, it is another task.

@Jim8y
Copy link
Contributor Author

Jim8y commented Jan 11, 2024

I find the issue and open the pr cause I need it now to debug the existing system, waiting for a release takes too long.

Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

We can add the description when it's available

@cschuchardt88
Copy link
Member

We can add the description when it's available

@shargon we should have a config file. Reason is for testing. https://api.github.com/repos/neo-project/neo-modules/releases is hard code. You know how easy it will be to test PRs. @vncoelho may want this configurable.

@shargon
Copy link
Member

shargon commented Jan 11, 2024

We can add the description when it's available

@shargon we should have a config file. Reason is for testing. https://api.github.com/repos/neo-project/neo-modules/releases is hard code. You know how easy it will be to test PRs. @vncoelho may want this configurable.

I hate hardcoded values, so for me It's good to move to settings.

@vncoelho
Copy link
Member

We can add the description when it's available

@shargon we should have a config file. Reason is for testing. https://api.github.com/repos/neo-project/neo-modules/releases is hard code. You know how easy it will be to test PRs. @vncoelho may want this configurable.

I hate hardcoded values, so for me It's good to move to settings.

Exactly, especially if the application has other plugins list

@Jim8y
Copy link
Contributor Author

Jim8y commented Jan 13, 2024

@shargon

@shargon
Copy link
Member

shargon commented Jan 13, 2024

We can improve it later, when we upload the manifest in modules

@shargon shargon merged commit a478329 into neo-project:master Jan 13, 2024
2 checks passed
Jim8y added a commit to Jim8y/neo that referenced this pull request Jan 13, 2024
…to specify-log-exception

* 'specify-log-exception' of github.com:Liaojinghui/neo:
  Update: list all existing pligins when checking plugin list, not just installed (neo-project#3080)
@roman-khimov roman-khimov added this to the v3.7.0 milestone Feb 27, 2024
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.

5 participants