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

Upgrade Mono completions #8452

Closed
wants to merge 28 commits into from
Closed

Upgrade Mono completions #8452

wants to merge 28 commits into from

Conversation

EmilyGraceSeville7cf
Copy link
Contributor

@EmilyGraceSeville7cf EmilyGraceSeville7cf commented Nov 17, 2021

Description

This PR upgrades mono.fish completions and adds completions for other Mono tools.

image

TODOs:

  • mono support
  • gacutil support
  • xsp support
  • mkbundle support
  • monodis support
  • ikdasm support
  • sqlsharp support
  • Gendarme support

@EmilyGraceSeville7cf
Copy link
Contributor Author

EmilyGraceSeville7cf commented Nov 19, 2021

I think I have to discuss (and maybe fix) raised problems with option parsing with Mono developers because they aren't not Fish shell problems.

@EmilyGraceSeville7cf
Copy link
Contributor Author

Until Mono developers fix mono/mono#21318 I can't write normal completions for some commands:

  • mcs
  • ilasm
  • monop
  • xsd
  • wsdl & wsdl2
  • disco
  • soapsuds

But of course basic completions will be created for them anyway (but in another PR later).

@EmilyGraceSeville7cf EmilyGraceSeville7cf marked this pull request as draft November 26, 2021 12:55
@EmilyGraceSeville7cf
Copy link
Contributor Author

I'll check these completions tomorrow and refresh them to better fit Fish code style.

@EmilyGraceSeville7cf EmilyGraceSeville7cf marked this pull request as ready for review January 25, 2022 10:56
share/completions/mono.fish Outdated Show resolved Hide resolved
share/completions/mono.fish Outdated Show resolved Hide resolved
share/completions/mono.fish Outdated Show resolved Hide resolved
complete -c mono -l ncompile \
-d 'Instruct the runtime on the number of times that the method(-s) specified by --compile/--compile-all to be compiled'
complete -c mono -l stats \
-d 'Display information about the work done by the runtime during the execution of an application'
Copy link
Member

Choose a reason for hiding this comment

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

how about putting each argument in a new line, for better readability?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
-d 'Display information about the work done by the runtime during the execution of an application'
complete -c mono \
-l ncompile \
-d 'Instruct the runtime on the number of times that the method(-s) specified by --compile/--compile-all to be compiled'
complete -c mono \
-l stats \
-d 'Display information about the work done by the runtime during the execution of an application'

Do you mean smth like that?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry the location of my comment is off (bug in my review script) it's about the --aot option (and other in that style)

So I meant that instead of

complete -c mono -l aot -f -a 'asmonly bind-to-runtime-version data-outfile direct-icalls'

I'd write

complete -c mono -l aot -f -a '
    asmonly
    bind-to-runtime-version
    data-outfile
    direct-icalls
'

though there are arguments for both styles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please check it now. ;)

share/completions/mono.fish Outdated Show resolved Hide resolved
share/completions/gacutil.fish Outdated Show resolved Hide resolved
share/completions/gacutil.fish Outdated Show resolved Hide resolved
share/completions/gacutil.fish Outdated Show resolved Hide resolved
share/completions/mkbundle.fish Outdated Show resolved Hide resolved
share/completions/mono.fish Show resolved Hide resolved
share/completions/gacutil.fish Outdated Show resolved Hide resolved
share/completions/mkbundle.fish Show resolved Hide resolved
@krobelus
Copy link
Member

Overall this is really great work!

- prefer short options
- fix types
- simplify quoting
- prefer builtin functions
@krobelus krobelus closed this in 1b12719 Feb 5, 2022
@krobelus
Copy link
Member

krobelus commented Feb 5, 2022

Merged as 1b12719, thanks!

@krobelus krobelus added this to the fish 3.4.0 milestone Feb 5, 2022
@EmilyGraceSeville7cf EmilyGraceSeville7cf deleted the mono-complete-upgrade branch February 5, 2022 19:12
@IlanCosman IlanCosman mentioned this pull request Feb 18, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants