Skip to content

feat: add option to skip gitignored files on git navigation #2583

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

Merged
merged 5 commits into from
Dec 19, 2023

Conversation

Akmadan23
Copy link
Collaborator

This would be handy since usually we don't want to focus gitignored files. We have to decide if this option should be local to the function (in this case we would need to add a couple of extra API wrappers, something like node.navigate.git.next_skip_gitignored) or if we want it to be global...

---@return fun()
function M.fn(where, what)
function M.fn(where, what, opts)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moreover, if we all agree, I'd do a little refactor here, having just an opts table with the where and what fields, so that we can easily add new ones if necessary.

Copy link
Member

@alex-courtis alex-courtis Dec 9, 2023

Choose a reason for hiding this comment

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

Yes please. The API could be altered to pass opts.

We would need to retain backwards compatibility as per ApiTreeToggleOpts so as not to break existing users.

If you are sufficiently motivated we could add enums for where and what :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unless we release 0.x.x and introduce whatever breaking changes we want for 1.0.

Copy link
Member

Choose a reason for hiding this comment

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

Unless we release 0.x.x and introduce whatever breaking changes we want for 1.0.

Interesting... the highlight overhaul could also go before 1.0.0. Perhaps we might do an audit and do any "final breaks" before the big release.

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

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

Looking good, skips nicely:

  • nowrapscan
  • changes before and after
  • changes before
  • changes after
  • skip_gitignored false

just need to get default mappings / API sorted.

---@return fun()
function M.fn(where, what)
function M.fn(where, what, opts)
Copy link
Member

Choose a reason for hiding this comment

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

Unless we release 0.x.x and introduce whatever breaking changes we want for 1.0.

Interesting... the highlight overhaul could also go before 1.0.0. Perhaps we might do an audit and do any "final breaks" before the big release.

@alex-courtis
Copy link
Member

alex-courtis commented Dec 10, 2023

What are you thinking about now for API? Maybe:

node.navigate.git.next({skip_gitignored = true})

or even full user control, not quite sure how it would play out:

node.navigate({ where = "prev", what = "git" , skip_gitignored = true})

I'm not the biggest fan of more options...

@Akmadan23
Copy link
Collaborator Author

node.navigate.git.next({skip_gitignored = true})

Between the two I would go with this one, but I would even prefer to add a separate node.navigate.git.next_skip_gitignored to easily use it in keymaps without a function() ... end wrapper...

@alex-courtis
Copy link
Member

Between the two I would go with this one, but I would even prefer to add a separate node.navigate.git.next_skip_gitignored to easily use it in keymaps without a function() ... end wrapper...

Fair enough; that's more consistent with the current no-arg navigation methods, indeed all of our default mappings. The user wouldn't really gain anything from the extra complication of args.

Skip ignored args are also very git specific and any other possible navigation skips would be equally specific.

Let's keep it simple.

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

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

Approved following the actual addition to API etc.

@Akmadan23
Copy link
Collaborator Author

We have a couple of choices to address this style issue:

  • Split lines on = like stylua suggests
  • Format tables line by line
  • Save require("nvim-tree.actions.moves.item") in a local variable

I'd personally go with the latter, but that would introduce an inconsistency with the rest of API entries. However, if we agree on this, I could work on a refactor to make it the standard. It would also greatly improve readability in my opinion...

@alex-courtis
Copy link
Member

alex-courtis commented Dec 19, 2023

We have a couple of choices to address this style issue:

  • Split lines on = like stylua suggests

  • Format tables line by line

  • Save require("nvim-tree.actions.moves.item") in a local variable

I'd personally go with the latter, but that would introduce an inconsistency with the rest of API entries. However, if we agree on this, I could work on a refactor to make it the standard. It would also greatly improve readability in my opinion...

It would be much more readable however we are in circular dependency land. API is a big problem.

  • Format tables line by line

Do you mean something like

Api.node = {
  navigate = {
    git = {
      ["next"] = wrap_node(require("nvim-tree.actions.moves.item").fn { where = "next", what = "git" }),
      prev = wrap_node(require("nvim-tree.actions.moves.item").fn { where = "prev", what = "git" }),
      next_skip_gitignored = wrap_node(require("nvim-tree.actions.moves.item").fn { where = "next", what = "git", skip_gitignored = true }),
      prev_skip_gitignored = wrap_node(require("nvim-tree.actions.moves.item").fn { where = "prev", what = "git", skip_gitignored = true }),
    },
  },
}

That would enhance readability.

There are a hundred ways we can refactor / split / tidy / lazy bind API however I'm happy enough with a -- stylua: ignore above the bindings. It's a special case.

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

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

Looking good; looking forward to using this one...

@Akmadan23
Copy link
Collaborator Author

There are a hundred ways we can refactor / split / tidy / lazy bind API however I'm happy enough with a -- stylua: ignore above the bindings. It's a special case.

Alright, let's merge this way for now.

It would be much more readable however we are in circular dependency land. API is a big problem.

Well, the api module is only required in the commands module, so there aren't many circular dependencies. I already worked on a refactor proposal, pushing PR soon.

@Akmadan23 Akmadan23 merged commit 50f30bc into master Dec 19, 2023
@Akmadan23 Akmadan23 deleted the skip-gitignored-on-git-navigation branch December 19, 2023 10:29
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.

3 participants