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

find-up #115

Closed
wants to merge 3 commits into from
Closed

find-up #115

wants to merge 3 commits into from

Conversation

eval
Copy link
Contributor

@eval eval commented Sep 27, 2023

Issue #112

Applied to clj-kondo.

considerations

  • currently raises on start not existing (as fs/parent happily traverses up existing or not) - maybe this should be configurable (seems needed for the clj-kondo-example that is currently not strict)
  • return file or path - currently going with path to be in line with e.g. glob
  • tests are very extensive - might be pruned

@eval eval marked this pull request as ready for review October 3, 2023 11:54
@eval
Copy link
Contributor Author

eval commented Oct 3, 2023

Removed draft, curious what you think @borkdude!

@borkdude
Copy link
Contributor

borkdude commented Oct 3, 2023

I'll have a look later this week!

@eval eval force-pushed the find-up-draft branch 3 times, most recently from 32f67e0 to a57a20d Compare October 6, 2023 12:45
@borkdude
Copy link
Contributor

borkdude commented Oct 7, 2023

Is there a reason why you keep force-pushing this branch? I prefer not to see force-pushes as it's hard to track incremental changes and to collaborate this way.

@eval
Copy link
Contributor Author

eval commented Oct 7, 2023

Is there a reason why you keep force-pushing this branch?

Mainline was changed, so I updated API.md - Sorry for the noise, I'll make new commits going fw.

@borkdude
Copy link
Contributor

borkdude commented Oct 7, 2023

ok, no problem, I'll get to this change soon, but have some other stuff to get through first, I haven't forgotten :)

@eval
Copy link
Contributor Author

eval commented Oct 7, 2023

Sure take your time - it was indeed not meant to 'poke' you, just to keep the patch in shape (I'm testdriving it in a project as well, so finding small improvements).

@borkdude
Copy link
Contributor

borkdude commented Oct 7, 2023

Ah thank you, yes, test-driving would be good!

@borkdude
Copy link
Contributor

borkdude commented Oct 9, 2023

I think this function does a bit too many things. E.g. the expand-home seems arbitrary, nowhere else in fs do we automatically expand home.

(cond-> x :alway(s) (foo))

can just be written as (cond-> (foo x) ...)

The logic seems a bit verbose and it's not immediately clear why expressions like required-start-folder-depth (count (filter #(= ".." %) (map str (normalize file)))) appear in the code, perhaps you can clarify this.

In the current state I'm hesitant to merge it until I find more use cases myself to test drive this function.

@borkdude
Copy link
Contributor

borkdude commented Oct 9, 2023

Perhaps there are other fs-like libraries that have a similar function we can obtain more information from.
One other thought: the search direction could perhaps be generalized? find-upwards, find-downwards? Not sure. I'd rather wait for more real use cases to appear, people are welcome to comment here or in the issue.

@eval
Copy link
Contributor Author

eval commented Oct 10, 2023

Thanks for taking a look. I agree it does a lot and this might all just remain a sketch...

That said, I took another look and pruned...:

  • removed the predicate-option, so it's just finding files, not filtering ancestors.
  • removed accepting "~"
    This is, as you said indeed, not supported elsewhere.
  • removed the checking for existence of root
    bogus paths in, nil out.
  • swapped arguments to match the signature of match and glob: [folder file]
  • improved the logic for below-root?
    To answer your question: This logic is needed as otherwise (find-up "/" "../file") could happily find /file due to how normalize works: (fs/normalize (fs/path "/../tmp")) is equivalent to (fs/path "/tmp"), as is (fs/path "/../../../../tmp").

I think the fn now has more focus and hope the remaining logic justifies this being included.

As to similar libs, there's this discussion where someone brought up https://github.com/rejeep/f.el.

@borkdude
Copy link
Contributor

As to similar libs, there's #56 where someone brought up https://github.com/rejeep/f.el.

I meant, a similar lib with a similar function as the one you're proposing. I'm not aware of any in f.el, Node's "fs" library etc, but happy to find out if there is one.

@eval
Copy link
Contributor Author

eval commented Jan 3, 2024

I meant, a similar lib with a similar function as the one you're proposing. I'm not aware of any in f.el, Node's "fs" library etc, but happy to find out if there is one.

There's https://www.npmjs.com/package/find-up (and https://www.npmjs.com/package/find-up-simple).

For some recent project I settled on traverse-up.

@frou
Copy link

frou commented Jan 22, 2024

Stock Emacs has the function locate-dominating-file. I guess it's called "dominating" because the presence of a recognisable filename can have a big influence in establishing context for what kind of project one is in.

@borkdude borkdude closed this Apr 14, 2024
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