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

added simple and ugly completion script for rgbasm #895

Merged
merged 2 commits into from
Oct 31, 2021

Conversation

DaKnig
Copy link
Contributor

@DaKnig DaKnig commented Jun 10, 2021

No description provided.

@Rangi42 Rangi42 linked an issue Jun 10, 2021 that may be closed by this pull request
Comment on lines 10 to 12
# using *.z80 -> if no .z80 files are found it uses literal "*.z80"
# using ./*.z80 would show options starting with an ugly ./
# this is a hard decision... maybe use sed to filter ./*?
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# using *.z80 -> if no .z80 files are found it uses literal "*.z80"
# using ./*.z80 would show options starting with an ugly ./
# this is a hard decision... maybe use sed to filter ./*?

(This comment appears outdated, since the above line does use sed -E "s_^./__.)

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 was trying to say "this is an ugly solution. but alternatives are equally ugly" you can remove it if you think it doesn't matter

@ISSOtm
Copy link
Member

ISSOtm commented Jun 13, 2021

Thanks for the contribution, DaKnig!

I'm noticing a few bugs (for example, rgbasm -W ▒ all with being the cursor, tabbing will complete all in a loop).

Also, tabbing rgbasm -E▒ (a no-argument short opt) with this script tabs a new word, whereas the zsh completion instead offers to complete more short options, as they can be coalesced into a single argument. I don't have a strong opinion on which one is better, and I don't think consistent behavior is necessary between both scripts as long as they match the standard for their respective shell. I must note that I have basically never used Bash completions, so I don't know about that.

The script also tabs -- to just - , and seems to forego long options entirely. Weird, I couldn't figure out why.

I gave a read to the documentation you linked and what that linked to, and I believe I now have a more solid understanding of how Bash completions work; thanks for that! I think that bugs can be fixed and some features improved by using builtins more (mainly complete itself, I think). I can do those modifications myself, if you don't want to, or just don't like writing Bash scripts :P

@DaKnig
Copy link
Contributor Author

DaKnig commented Jun 13, 2021

I was comparing against other native Unix tools completion with bash. IMO the stacked short opt completion is not necessary, a user should be able to just make many one letter opts. not just for consistency with behavior with some other programs but also for simplicity of the script.
about the -W all loop, idk how to easily solve it. I also don't know what other tools have similar flag structure so we can compare the behavior, not sure this is an unwanted behavior.
regarding using more builtins, I surely agree, I think it should still be done mostly in a function so that improvements are easier to make, so whatever we end up pushing should IMO use those builtins inside the function.

I think the time I had allocated for this project is over, and I can't stand coding in bash for much longer, so I won't be able to finish the job... maybe we can do this in C? or is maintaining C a hassle? maybe launching C just for this would be extremely slow? maybe python can do this better.. I am not sure

Copy link
Contributor Author

@DaKnig DaKnig left a comment

Choose a reason for hiding this comment

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

now the code inside the body of the if statement doesn't have one indentation over what the if has

@Rangi42 Rangi42 added enhancement Typically new features; lesser priority than bugs meta This isn't related to the tools directly: repo organization, maintainership... labels Jun 20, 2021
@ISSOtm
Copy link
Member

ISSOtm commented Oct 31, 2021

Rewrote the script to fix the whitespace bugs, and more properly parse the command-line! Hopefully that'll do better.

@Rangi42 Rangi42 added this to the v0.5.2 milestone Oct 31, 2021
README.rst Outdated Show resolved Hide resolved
@ISSOtm ISSOtm force-pushed the bash_completion branch 2 times, most recently from d0fa0fd to 2509aa9 Compare October 31, 2021 22:21
Should have large feature parity with the Zsh completion

Co-authored-by: DaKnig <[email protected]>
@ISSOtm ISSOtm merged commit b16d2d0 into gbdev:master Oct 31, 2021
@DaKnig DaKnig deleted the bash_completion branch November 1, 2021 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Typically new features; lesser priority than bugs meta This isn't related to the tools directly: repo organization, maintainership...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bash completion scripts
3 participants