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

refactoring of README.md #1601

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gennaro-tedesco
Copy link
Contributor

@gennaro-tedesco gennaro-tedesco commented Dec 19, 2024

I have seen this comment of yours on reddit and have decided to come up with a proposal to help you clear up the docs. Let me try to explain the rationale of this PR before you get a heart attack by looking at the diff :D

I have been a long-term Fzf-lua user and more or less know my way around this project, in the sense that I know where to find information I need; however, I sometimes do struggle navigating through the README, the Wiki and the issues just because of course there are so many options and contributions and it is hard to have a unified glance. For this reason I have proposed a strong refactoring of the README based on the below considerations:

  1. the vast majority of users, the vast majority of times (say 90% of users 90% of times) exclusively look for configuration options, hence such must be the impact focus of the README, easily reachable, easily browsable. I have moved sections around to focus on Options first and foremost.
  2. Installation and configuration instructions nowadays are done in lua only, I have removed all the bits that contain vimscript instructions (they are relevant for a minority of users only)
  3. I have folder most if not all sections into <details> expandables: the README is very long and it cannot be browsed otherwise
  4. I have separated the configuration options into different (foldable) sections according to the type: at the moment there is one long list that is difficult to search through
  5. I have removed the table of contents because GitHub provides it already by itself (top right of the README rendering), so that we can save some "visual space"

As I said, this is a strong refactoring of the README and it serves as proposal only, I did not mean to intentionally remove instructions or else, only instead to provide a basis for thoughts in case you would like to offer a leaner documentation (especially for first time users).

README diffs are hard to read, to see what it looks like directly in the browser hop on the fork and check it out.

README.md Outdated
have been many obscure requests which have been fulfilled and are yet to be
documented. If you're still having issues and/or questions do not hesitate to open an
issue and I'll be more than happy to help.**
where each of the above sections can be fully configured as below:
Copy link

@vunhatchuong vunhatchuong Dec 19, 2024

Choose a reason for hiding this comment

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

I think this should explicitly mention that these are the default options since that is one of the most important aspects of a plugin.

Maybe something like: "Each of the above sections can be fully configured, with the following settings serving as the default options:" so that people can also search for the keyword default.

@ibhagwan
Copy link
Owner

ibhagwan commented Dec 19, 2024

before you get a heart attack by looking at the diff :D

Disclaimer noted, although at first glance I really like where this is going and this has been long overdue.

I have been a long-term Fzf-lua user and more or less know my way around this project

We've conversed enough times before that I wouldn't forget you this quickly :-)

As I said, this is a strong refactoring of the README and it serves as proposal only, I did not mean to intentionally remove instructions or else, only instead to provide a basis for thoughts in case you would like to offer a leaner documentation (especially for first time users).

I agree with all of your suggessions, we can work on this together until this gets to a point where everyone is happy, much appreciate the effort and thought you put into this.

README diffs are hard to read, to see what it looks like directly in the browser hop on the fork and check it out.

I have a custom css that renders the markdown very closely to GitHub so I can also view this offline in my browser:

https://github.com/ibhagwan/nvim-lua/blob/af41cafd37d962984b3f54fbf0d4fe58eb065f94/lua/plugins/init.lua#L36-L62

image

But since I added render-markdown.nvim (also very cool), I use it less:
image

@milanglacier
Copy link

milanglacier commented Dec 20, 2024

The current documentation does not clearly state the behavior when user has passed his custom keybindings. The behavior is that when user has passed the custom keybindings, then all the default keybindings will be removed. To keep the default the keybindings, the user must set the first key to true.

Currently the documentation states that:

require('fzf-lua').setup {
    actions = {
        -- Below are the default actions, setting any value in these tables will override
        -- the defaults, to inherit from the defaults change [1] from `false` to `true`
        files = {
            false, -- do not inherit from defaults
        },
    },

}

However, this statement is confusing. What does "Inherit from the defaults" mean? "Default" things are what should be inherited. "Default" and "inheritance" are synonymous in my mind. So you can't inherit a default, you can clear the defaults entirely, or override part of the defaults.

So in order to state it clearly, I personally think if we state the behavior in an opposite direction, that should be clearer, aka something like this: If you pass any custom keybindings here, all the default keybindings will be cleared. If you want to keep using the default keybindings with your custom keybindings, you should set the first key to true

See the complete discussion at here:

#1612 (reply in thread)

@ibhagwan
Copy link
Owner

FYI, rebased to main so fork is up-to-date on the latest commits.

ibhagwan added a commit to gennaro-tedesco/fzf-lua that referenced this pull request Dec 23, 2024
@ibhagwan
Copy link
Owner

Added my 2 cents on this refactor, restored the "quickstart" to the top (I think it's important) and made other modifications, categories for commands, etc, see the end result in @gennaro-tedesco's fork

Comments ans suggesions welcome

ibhagwan added a commit to gennaro-tedesco/fzf-lua that referenced this pull request Dec 23, 2024
ibhagwan added a commit to gennaro-tedesco/fzf-lua that referenced this pull request Dec 23, 2024
@gennaro-tedesco
Copy link
Contributor Author

Another section that I believe we should improve is the lsp pickers options, in particular I have been browsing myself a few issues (here here and here) to understand how to filter by symbol; likewise with the options query and lsp_query (many questions about them in the discussion section).

I am not sure myself yet how to use such options, hence I would not want to add some wrong documentation. My understanding so far is that lsp_query is the query that is passed to nvim API vim.lsp.buf.workspace_symbol(query=<lsp_query>) and whose results are independent of fzf-lua (what the LSP returns or not in such cases depends exclusively on the LSP and how it indexes the project symbols). On the other hand, query is the fuzzy search query which one may want to pass as argument to somehow already open the picker and filter for some fuzzy terms. Is this understanding correct?

@ibhagwan
Copy link
Owner

I am not sure myself yet how to use such options, hence I would not want to add some wrong documentation. My understanding so far is that lsp_query is the query that is passed to nvim API vim.lsp.buf.workspace_symbol(query=<lsp_query>) and whose results are independent of fzf-lua (what the LSP returns or not in such cases depends exclusively on the LSP and how it indexes the project symbols). On the other hand, query is the fuzzy search query which one may want to pass as argument to somehow already open the picker and filter for some fuzzy terms. Is this understanding correct?

Correct, query always the fuzzy march query used by fzf, lsp_query is similar to the search/regex in grep as every new input changes the underlying LSP API query (like live grep).

@gennaro-tedesco
Copy link
Contributor Author

I have pushed a little commit adding the above explanation to the lsp options :)

ibhagwan added a commit to gennaro-tedesco/fzf-lua that referenced this pull request Dec 23, 2024
@ibhagwan
Copy link
Owner

I have pushed a little commit adding the above explanation to the lsp options :)

I'm not sure about this change, first the query/lsp_query should not appear as these are part of the defaults, second, the default value is nil and not an empty string and I still don't find the explanaiton clear, I'll think of something.. left it commented for now.

ibhagwan added a commit to gennaro-tedesco/fzf-lua that referenced this pull request Dec 23, 2024
@ibhagwan ibhagwan force-pushed the main branch 2 times, most recently from 7eaf854 to 8595c60 Compare December 24, 2024 22:44
gennaro-tedesco and others added 3 commits December 24, 2024 14:46
additional folding of second-order paragraphs

fixed anchors on top of README

split general options into different foldables

added config section for hls

fixed reference in table of contents

Proposed refactoring of README.md
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.

4 participants