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.

Sign up for GitHub

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

Jump to bottom

Make sure that list of plugins is sorted and unique #5395

Merged
merged 15 commits into from
Jan 3, 2024
Merged

Make sure that list of plugins is sorted and unique #5395

merged 15 commits into from
Jan 3, 2024

Conversation

crodas
Copy link
Contributor

@crodas crodas commented Dec 15, 2023 •
edited
Loading

Description

Fixes #5396, #5402, #528

The sorting is performed by the file_name instead of the full text to be printed or full path

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@crodas
Make sure that list of plugins is sorted and unique
d94930d 
The sorting is performed by the file_name instead of the full text to be
printed or full path
@crodas crodas requested a review from a team December 15, 2023 12:29
@crodas crodas self-assigned this Dec 15, 2023
@crodas crodas marked this pull request as ready for review December 15, 2023 12:29
@crodas crodas enabled auto-merge (squash) December 15, 2023 12:34
kayagokalp
kayagokalp reviewed Dec 15, 2023
View reviewed changes
Copy link
Member

@kayagokalp kayagokalp left a comment

Choose a reason for hiding this comment

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

I haven't tested it locally but looks good! One small suggestion about the error handling so that the output is more understandable on our side, if something happens.

forc/src/cli/commands/plugins.rs Show resolved Hide resolved
@kayagokalp kayagokalp requested a review from a team December 15, 2023 18:01
@kayagokalp kayagokalp added forc enhancement New feature or request labels Dec 15, 2023
sdankel
sdankel reviewed Dec 16, 2023
View reviewed changes
forc/src/cli/commands/plugins.rs Outdated Show resolved Hide resolved
forc/src/cli/commands/plugins.rs Outdated Show resolved Hide resolved
@crodas crodas requested review from sdankel and a team December 16, 2023 02:55
@crodas crodas mentioned this pull request Dec 18, 2023
Closed
7 tasks
@crodas crodas marked this pull request as draft December 18, 2023 23:35
auto-merge was automatically disabled December 18, 2023 23:35

Pull request was converted to draft

@crodas
Merge remote-tracking branch 'origin/master' into remove-dup-plugins
d91a8b5
@crodas
Print warning if binaries are not in the same folders
440d32a 
If the `forc` and the underlying binary for the plugin are not in the
same path *and* the `--verbose` flag is being passed to `forc` a warning
will be printed to stderr.

This may show a warning to stderr:

```
forc --verbose tx
```

This will never show a warning:

```
forc tx
```
@crodas
Copy link
Contributor Author

crodas commented Dec 19, 2023

Here is a video showing the new functionality added in 440d32a

Screen.Recording.2023-12-19.at.02.11.10.mov

@crodas crodas marked this pull request as ready for review December 19, 2023 05:47
@crodas crodas enabled auto-merge (squash) December 19, 2023 05:48
sdankel
sdankel reviewed Dec 19, 2023
View reviewed changes
forc/src/cli/plugin.rs Outdated Show resolved Hide resolved
forc/src/cli/commands/plugins.rs Outdated Show resolved Hide resolved
forc/src/cli/plugin.rs Outdated Show resolved Hide resolved
kayagokalp
kayagokalp reviewed Dec 29, 2023
View reviewed changes
Copy link
Member

@kayagokalp kayagokalp left a comment

Choose a reason for hiding this comment

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

Left a single nit and a question, looking good!

forc/src/cli/commands/plugins.rs Outdated Show resolved Hide resolved
forc/src/cli/plugin.rs Show resolved Hide resolved
forc/src/cli/commands/plugins.rs Outdated Show resolved Hide resolved
forc/src/cli/commands/plugins.rs Outdated Show resolved Hide resolved
@crodas crodas requested review from kayagokalp and a team December 29, 2023 17:42
kayagokalp
kayagokalp previously approved these changes Dec 29, 2023
View reviewed changes
Copy link
Member

@kayagokalp kayagokalp left a comment

Choose a reason for hiding this comment

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

🚀

@JoshuaBatty
Copy link
Member

JoshuaBatty commented Jan 2, 2024 •
edited
Loading

When running locally I now get this.

~ % forc plugins --paths
Installed Plugins:
/Users/joshuabatty/.cargo/bin/forc-doc
/Users/joshuabatty/.cargo/bin/forc-lsp

I would expect it should still show all of the forc tools? i.e forc-tx forc-fmt etc...

@crodas crodas merged commit caa1822 into master Jan 3, 2024
33 checks passed
@crodas crodas deleted the remove-dup-plugins branch January 3, 2024 05:47
@crodas crodas mentioned this pull request Jan 3, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdankel sdankel sdankel approved these changes

@JoshuaBatty JoshuaBatty JoshuaBatty approved these changes

@kayagokalp kayagokalp Awaiting requested review from kayagokalp

Assignees

@crodas crodas

Labels
enhancement New feature or request forc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

forc plugins may list plugins multiples
4 participants
@crodas @JoshuaBatty @kayagokalp @sdankel