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

feat: refactors transform_path and allow smart to be used in conjunction with filename_first #3152

Merged

Conversation

alycklama
Copy link
Contributor

@alycklama alycklama commented Jun 1, 2024

Description

I've cleaned up transform_path:

  • Extracted code into functions: path_filename_first, path_truncate, etc.
  • By returning early for smart we would not be able to run filename_first afterwards.

Smart pre-PR

image

Smart post-PR

The directory can now be separated with the following configuration:

path_display = {
  "smart",
  "filename_first",
},
image

Type of change

  • Feature: allowing smart to be used in conjunction with filename_first

How Has This Been Tested?

  • Local tests are green
  • Ran Telescope in Neovim locally

Configuration:

  • Neovim version (nvim --version):

NVIM v0.10.0
Build type: Release
LuaJIT 2.1.1716656478

  • Operating system and version:

OSX Sonoma 14.4.1

Checklist:

  • My code follows the style guidelines of this project (stylua)
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (lua annotations)

lua/telescope/utils.lua Outdated Show resolved Hide resolved
lua/telescope/utils.lua Outdated Show resolved Hide resolved
lua/telescope/utils.lua Outdated Show resolved Hide resolved
@alycklama alycklama changed the title Refactors transform_path and allow smart to be used in conjunction with filename_first feat: refactors transform_path and allow smart to be used in conjunction with filename_first Jun 2, 2024
@alycklama alycklama requested a review from Conni2461 June 2, 2024 12:45
lua/telescope/utils.lua Outdated Show resolved Hide resolved
lua/telescope/utils.lua Outdated Show resolved Hide resolved
lua/telescope/utils.lua Outdated Show resolved Hide resolved
lua/telescope/utils.lua Outdated Show resolved Hide resolved
lua/telescope/utils.lua Outdated Show resolved Hide resolved
@alycklama
Copy link
Contributor Author

@Conni2461 I left a comment related to one of your suggestions. Can you let me know what you favor yourself? I'll do follow up based on your reply. Thanks!

lua/telescope/utils.lua Outdated Show resolved Hide resolved
lua/telescope/utils.lua Outdated Show resolved Hide resolved
lua/telescope/utils.lua Outdated Show resolved Hide resolved
lua/telescope/utils.lua Outdated Show resolved Hide resolved
lua/telescope/utils.lua Outdated Show resolved Hide resolved
@alycklama alycklama requested a review from Conni2461 June 10, 2024 08:04
lua/telescope/utils.lua Outdated Show resolved Hide resolved
Copy link
Member

@Conni2461 Conni2461 left a comment

Choose a reason for hiding this comment

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

general feedback, you should start looking at your diffs after you've submitted them. this took way too many iterations and so far I've only ever looked over it superficially

@alycklama
Copy link
Contributor Author

general feedback, you should start looking at your diffs after you've submitted them. this took way too many iterations and so far I've only ever looked over it superficially

Thank you for the feedback! I'll go through the commits more carefully.

There's some feedback on my side as well, though. As every codebase is different, it takes some time to understand what the maintainers expect. If I see a public method utils.path_smart in a utils class, I don't have the context to understand this was a mistake and should never have been made public to begin with. To me it was just that. A util class with public methods. It would be great to document these "mistakes" in the code, so people have the context directly from reading through the code. A comment above utils.path_smart saying -- TODO: This should never have been exposed would be truly valuable for anyone trying to refactor the code.

@Conni2461
Copy link
Member

Can you add a comment like that (i think there are more functions in that file but i cant check right now, i am on mobile and tbh it doesnt matter), ill test that change tomorrow, after that, its good to go. Thanks :)

@alycklama
Copy link
Contributor Author

Thanks! @Conni2461

I've checked all the methods and added a comment stating why it's public, even though it's only used within the utils file itself.

One question, though. I noticed that there are 2 variants that look the same to me:

-- Variant 1
function utils.hello_world()
end

-- Variant 2
utils.hello_world = function()
end

They seem to do the exact same thing, but with a different flavour of how it's written. I've tested this with the code below in and the function is callable:

local utils = {}

function utils.hello_world()
	print("hello world!")
end

utils.hello_world()

Just wanted to check with you whether these are indeed just flavours of the same thing.

@alycklama alycklama requested a review from Conni2461 June 14, 2024 15:12
@Conni2461
Copy link
Member

thanks :)

@Conni2461 Conni2461 merged commit 25f0450 into nvim-telescope:master Jun 15, 2024
12 checks passed
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