Lighter-swift / Lighter

Swift APIs for SQLite: Type-safe down to the schema. Very, very, fast. Dependency free.
https://lighter-swift.github.io/documentation/lighter/
MIT License
458 stars 12 forks source link

Slightly improved heuristic to find the target root #24

Closed micampe closed 1 year ago

micampe commented 1 year ago

Since I don't have any files in my target's root directory (only other subdirectories), the current heuristic can't find my sql file, because it only looks at those subdirectories, so it picks a random one (the first in the list).

I could work around it but moving a file there but I propose a new heuristic that solves the problem by finding the longest common path in the input files directories. Implementation inspired by Python's os.path.commonpath().

This is obviously doing a bit more work than before but I don't think it should cause a performance problem – most of the added work is sorting the directories, and I only sort them by string length.

micampe commented 1 year ago

Thinking about it as a more general solution, considering that the files could be anywhere, there could be no common path at all, which would return / or /Users/username, and doesn't make sense.

helje5 commented 1 year ago

Since I don't have any files in my target's root directory (only other subdirectories), the current heuristic can't find my sql file, because it only looks at those subdirectories

I think the subdirectories shouldn't matter? The inputFiles should contain ALL files that are part of the target, regardless where they live, even in subfolders?

Like say:

Project/
  Target/
    Subdir/
      A.swift
      Further/
        B.swift

It looks at "Subdir" and "Further" for "/Target", neither matches. So I think the modification needs to check the dirs whether they contain Target at a higher level, and then maybe use the longest path.

I.e. instead of just doing hasSuffix("/Target"), so .contains("/Target/") as a fallback.

I think the proposed patch is incorrect, as it would just always pick the longest subdir, regardless of whether it contains the name we are looking for. I.e. it fails this:

Project/
  Target/
    A.swift
    Subdir/
      Further/
        B.swift

which currently works as expected

helje5 commented 1 year ago

Do you want to give this another try, or should I give it a shot?

micampe commented 1 year ago

I agree the patch is incorrect, but your second example works: the directories array would contain

Project/Target
Project/Target/Subdir/Further

and since it looks for the longest path prefix (not just the longest path) it finds Project/Target.

It fails when the target contains a file that is outside of its root directory, for example if Outside.swift here is part of TargetB (by using an absolute path, for example), my patch returns Project, which is wrong.

Project/
  TargetA/
    Outside.swift
Project/
  TargetB/
    Inside.swift

I'll try again with your suggestion of searching the longest path containing /Target/.

micampe commented 1 year ago

never mind, still wrong :)

micampe commented 1 year ago

ok, I think it works now. I'm not too happy with how complicated it is but I couldn't find a way to simplify it. If you have a better idea feel free to take over.

I also tweaked some of the printed messages that would have helped me to diagnose my problem.

micampe commented 1 year ago

New patch that I think is clearer.