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

General improvements #103

Closed
wants to merge 6 commits into from
Closed

Conversation

marcdeop
Copy link
Contributor

I just wanted to fix a small issue I had but I got carried away and somehow ended up with a bigger PR.

What this PR does:

New variable

Introduces extrakto_fzf_unset_default_opts in order to unset the environment variable FZF_DEFAULT_OPS.

I found out that certain operations would not work with my default FZF_DEFAULT_OPS value.

For example:
Having the option -1 (or --select-1) will make extracto's pop-up disappear immediately when there is 0 or just 1 match thus preventing you from switching to another filter.

Fix and simplify filter modes validation

This is what I wanted to fix originally.

Even though extrakto_filter_orderexists, if you try to use another filter like url it will just not work.

Now this is fixed:

valid_modes=("word" "all" "line" "url" "path" "quote" "s-quote")

I also converted this into a standard array rather than an associative array because it makes things simpler and easier.

Support other modes

Extra modes added:

  • path
  • url
  • quote
  • s-quote

Cosmetics

Some linting with quoting, remove unnecessary echo and add --word as a parameter for the sake of consistency.

How to try?

Before this PR:

set -g @extrakto_filter_order "url line quote s-quote word path all"

And no matter what you did, the filters would always be: word all line

Now calling extracto will give you the url filter as default as expected.

@laktak
Copy link
Owner

laktak commented Feb 28, 2023

Thank you for your contributions! 👍 Just so you know - it will take me some time to review them.

@marcdeop
Copy link
Contributor Author

Thank you for your contributions! +1 Just so you know - it will take me some time to review them.

all good :-)

In general I am confident. The only thing we might need to revert back is the valid_modes again to an associative array if it doesn't work properly.

Looking forward to your feedback!

@laktak
Copy link
Owner

laktak commented Sep 21, 2023

Sorry for leaving this open for so long.

I use extrakto a lot and it's quite stable. I do not like to accept changes that change internals without any benefit other than making it look nice internally.

If you are still interested, could you try a smaller PR first? Thanks

@marcdeop
Copy link
Contributor Author

Sorry for leaving this open for so long.

I use extrakto a lot and it's quite stable. I do not like to accept changes that change internals without any benefit other than making it look nice internally.

If you are still interested, could you try a smaller PR first? Thanks

I could just have removed the parts you consider too invasive, you know? That's why several commits were made.

Would you mind clarifying what exactly was changing the internals in a way you don't like? the echo? the array?

@laktak
Copy link
Owner

laktak commented Sep 22, 2023

OK, there may be some miscommunication. Sorry if this didn't come across as intended.

Closing this PR does not mean I don't appreciate your efforts. Starting a new PR with smaller changes is IMO a better workflow than picking commits here, so that's why I closed it.

I think the changes to extrakto.py and the ones you labeled "cosmetic" are unnecessary.

Do the changes work with Bash 5.0? Could you put the "Fix and simplify filter modes validation" into a new PR?

@marcdeop marcdeop mentioned this pull request Oct 21, 2023
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.

2 participants