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

Add bash completions #62

Merged
merged 6 commits into from
Jan 17, 2025
Merged

Add bash completions #62

merged 6 commits into from
Jan 17, 2025

Conversation

TobiPeterG
Copy link
Collaborator

@TobiPeterG TobiPeterG commented Mar 5, 2024

This will complete started commands and provide a list of possible commands and options.

Closes #61

Unfortunately, we can't access the available snapshots, so the user still has to know which ones are available on his system. Equally, we can't enter snapshots to find kernel versions, so we recommend only the currently installed kernels.

@lnussel
Copy link
Member

lnussel commented Mar 5, 2024

I'd probably add a hidden function to sdbootutil that can be eval'd from the completion script to avoid duplicating all that code

@TobiPeterG
Copy link
Collaborator Author

I'd probably add a hidden function to sdbootutil that can be eval'd from the completion script to avoid duplicating all that code

I've looked a bit into how bash completion works and it seems like the bash completion has to happen in the same shell context. So I can A) source the sdbootutil script, but that would execute the functions that might fail as the bash completion can be called as non-super user
B) call a function in sdbootutil where I echo the recommendations, but that would also not be a nice solution
C) Create two functions in sdbootutil that are callable that return the architecture/image name and kernels in the current snapshot.

Or do you have a different idea?
I agree that the code duplication isn't nice

@lnussel
Copy link
Member

lnussel commented Mar 5, 2024

something like eval $(sdbootutil _print_bash_completion_data)

@TobiPeterG TobiPeterG force-pushed the bash-completion branch 3 times, most recently from a5d7b01 to c4d47d4 Compare March 5, 2024 17:37
@TobiPeterG
Copy link
Collaborator Author

something like eval $(sdbootutil _print_bash_completion_data)

I've tried to implement this. I couldn't test it yet though, but please tell me if this is what you had in mind :)
The code also isn't nice yet, it's just thought as a concept currently :)

sdbootutil Outdated Show resolved Hide resolved
sdbootutil Outdated Show resolved Hide resolved
sdbootutil Outdated Show resolved Hide resolved
sdbootutil Outdated Show resolved Hide resolved
@TobiPeterG TobiPeterG force-pushed the bash-completion branch 4 times, most recently from a93f9a9 to 9e50d0c Compare March 7, 2024 00:41
sdbootutil Outdated Show resolved Hide resolved
sdbootutil Outdated Show resolved Hide resolved
sdbootutil Outdated Show resolved Hide resolved
sdbootutil Outdated Show resolved Hide resolved
@TobiPeterG TobiPeterG force-pushed the bash-completion branch 3 times, most recently from da4439b to 7e20590 Compare March 21, 2024 21:58
@TobiPeterG TobiPeterG requested a review from lnussel March 21, 2024 21:59
completion/sdbootutil Outdated Show resolved Hide resolved
sdbootutil Outdated Show resolved Hide resolved
sdbootutil Outdated Show resolved Hide resolved
sdbootutil Outdated Show resolved Hide resolved
completion/sdbootutil Outdated Show resolved Hide resolved
completion/sdbootutil Outdated Show resolved Hide resolved
@TobiPeterG TobiPeterG force-pushed the bash-completion branch 3 times, most recently from 92f03a7 to b64cdb5 Compare April 9, 2024 18:01
@TobiPeterG TobiPeterG requested a review from lnussel April 9, 2024 19:39
Copy link
Member

@lnussel lnussel left a comment

Choose a reason for hiding this comment

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

almost there

sdbootutil Outdated Show resolved Hide resolved
@TobiPeterG TobiPeterG force-pushed the bash-completion branch 4 times, most recently from 40a7578 to 588ed4b Compare May 20, 2024 13:35
@TobiPeterG TobiPeterG requested a review from lnussel May 20, 2024 13:37
@TobiPeterG
Copy link
Collaborator Author

@aplanas can you have a look at this one please? :)

@TobiPeterG TobiPeterG force-pushed the bash-completion branch 2 times, most recently from 5b8c2a1 to 2857228 Compare July 5, 2024 15:12
@TobiPeterG TobiPeterG requested a review from aplanas July 8, 2024 09:51
@aplanas
Copy link
Collaborator

aplanas commented Jul 8, 2024

@TobiPeterG the PR is looking good. Give me some time to test it a bit

@TobiPeterG
Copy link
Collaborator Author

Should I just rebase or do you have any issues with the functionality/code that I should fix with a rebase?

@aplanas
Copy link
Collaborator

aplanas commented Jul 31, 2024

Yes please, do a rebase. But I need to research a bit more. I found a good reference

Copy link
Collaborator

@aplanas aplanas left a comment

Choose a reason for hiding this comment

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

I think we can split this PR is two: one for all the bash completions, but another one for the commands refactoring. This last part is the main source of conflicts right now

sdbootutil Show resolved Hide resolved
sdbootutil Outdated Show resolved Hide resolved
@TobiPeterG
Copy link
Collaborator Author

Now depends on #172

I have refactored this PR and split it into multiple commits. Please have a look :)

@TobiPeterG
Copy link
Collaborator Author

To also support snapshots as completion, it would be the easiest to add a new option "--snapshot" that I can check for i nthe completion script. Otherwise it's impossible to know if the user wants to add an option or a snapshot number and I don't want to show both in one list.

@TobiPeterG
Copy link
Collaborator Author

Rebased :)

I added the "_none" function when no completion is useful.

@TobiPeterG
Copy link
Collaborator Author

@aplanas Rebased :)

sdbootutil Outdated Show resolved Hide resolved
sdbootutil Outdated Show resolved Hide resolved
Copy link
Collaborator

@aplanas aplanas left a comment

Choose a reason for hiding this comment

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

I would merge it after the commented issue

Copy link
Collaborator

@aplanas aplanas left a comment

Choose a reason for hiding this comment

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

Sorry that this takes so long : (

And thanks!

@aplanas aplanas dismissed lnussel’s stale review January 17, 2025 18:19

Many changes after this request of change

@aplanas aplanas merged commit efef718 into openSUSE:main Jan 17, 2025
@TobiPeterG
Copy link
Collaborator Author

Sorry that this takes so long : (

And thanks!

All good :)

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.

Enhancement: Add bash completions
3 participants