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

dependency doesn't track path below base directory #103

Closed
dkebler opened this issue Feb 9, 2015 · 11 comments · Fixed by #105
Closed

dependency doesn't track path below base directory #103

dkebler opened this issue Feb 9, 2015 · 11 comments · Fixed by #105
Labels

Comments

@dkebler
Copy link
Contributor

dkebler commented Feb 9, 2015

If one of my imports directs to a code file in subdirectory below the set "root" directory and then that file contains yet another import. Jumping to that dependency the plugin is unaware that it is in a subfolder and should include that subfolder in the path. Instead it looks back in the root. I confirmed this by simply moving the dependency file to the root.

In the console output below _base-theme.scss is in the subdirectory /base (@import 'base/base-theme') as is _color.scss, but the plugin is not looking in /base but rather the scss root I set which was "framework/scss".

I have a copy of what I am working on at github. You can see for yourself. Take a look at the path structure of my scss there.
https://github.com/dkebler/hugo-quickstart-site/tree/master/framework/scss
sitestyle.scss is the root dependent.

Not sure I would call this a bug so much as an enhancement, but organizing code guidelines encourage using subdirectories and thus import paths are relative. Seems like you have to grab the open file's path and compare it to the root set path and strip out and insert any subdirectory path(s) for use. Dependants will be a problem too. Right now that only works only because I am one subdirectory deep. Guess you could keep stripping out subdirectories looking for the file until you arrive at the root. Searching the entire tree at once would not be a good idea since identical file names could be used in different path branches (and I do!) although if you hit the root and don't find it then other branches could be searched.

filename:  /mnt/AllData/DGK/hacking/hugosites/testsite/framework/scss/base/_base-theme.scss
JumpToDependency: Extracted Path {'path': 'color'}
JumpToDependency: Extension found 
JumpToDependency: Before abs path resolution color.scss
JumpToDependency: After abs path resolution /mnt/AllData/DGK/hacking/hugosites/testsite/framework/scss/color.scss
ext:  .scss
filename:  /mnt/AllData/DGK/hacking/hugosites/testsite/framework/scss/color.scss
JumpToDependency: Now looking for underscored sass path
JumpToDependency: Underscored file: /mnt/AllData/DGK/hacking/hugosites/testsite/framework/scss/_color.scss
JumpToDependency: Opening: /mnt/AllData/DGK/hacking/hugosites/testsite/framework/scss/_color.scss
Missing file {'filename': '/mnt/AllData/DGK/hacking/hugosites/testsite/framework/scss/_color.scss', '_filename': 'JumpToDependency.py'}
error: Dependents

Can't find that file
Run_JumpToDependency {'etime': '0.2', '_filename': 'JumpToDependency.py'}

@mrjoelkemp mrjoelkemp added the bug label Feb 9, 2015
@mrjoelkemp
Copy link
Collaborator

Great find @dkebler. Thanks for contributing.

This is import resolution behavior that differs from JavaScript-centric dependency loaders. This is definitely a bug; my hunch is that both sublime-dependents and node-dependents will need this fix.

I'll ping you when it's done. Thanks again!

@dkebler
Copy link
Contributor Author

dkebler commented Feb 9, 2015

Along this line for those using bower/compass like I do there i have @imports that reference paths in the config.rb used by bower.

So for example if look at my _packages.scss
https://github.com/dkebler/hugo-quickstart-site/blob/master/framework/scss/vendors/_packages.scss
you will see no path in the imports there because compass looks for those paths in its config.rb.

So your package to to be useful in the sense of being able to follow a sass dependency into an "external" package (outside the root) you'd have to parse the config.rb file.
https://github.com/dkebler/hugo-quickstart-site/blob/master/framework/config.rb
for those paths to see if the dependency is there before saying it's not found.

I guess that would mean another setting which would be path to compass config.rb file. If set then you'd know the person was using compass.

I don't use grunt or other sass compilers but I imagine it's a similar issue.

@mrjoelkemp
Copy link
Collaborator

@dkebler Awesome suggestion. I've made it as another issue: #104. Supporting compass_config is the way to go.

Thanks!

@mrjoelkemp
Copy link
Collaborator

@dkebler This was fixed in version 2.4.6 (submitted to package_control and pending release within the hour). The lookup order is detailed here: https://github.com/mrjoelkemp/sublime-dependents/releases/tag/v2.4.6.

I have to make sure this bug doesn't affect the Find Dependents feature.

Thanks again for contributing.

@dkebler
Copy link
Contributor Author

dkebler commented Feb 11, 2015

With latest version I can now drill down dependencies multiple
layers(directories) deep. That seems to be working.

But
Dependents ONLY works looking for a dependent at the root from one layer
deep.
Finding dependents within the same subdirectory of the open file fails
(except in root directory)
as well as finding dependent when open file two or more subdirectories
deep.

As an example. Drill down from the sass root, main file of my repo
(framework/scss/sitestyle.scss), to the 'base/base-theme then to 'layouts'
then to 'layout/page' (works), but try going back (dependents) from any of
those files and it will only work from 'base-theme'.
https://github.com/dkebler/hugo-quickstart-site/blob/master/framework/scss/sitestyle.scss

Just for grins I put a 'test' dependency in the /base/layout/_page.scss
file that looks up one directory '../_test.scss'. That works but
surprisingly from that file base/_test.scss it finds the _page.scss
dependent one subdirectory down! This is an usual construction despite
working.

full repo here
https://github.com/dkebler/hugo-quickstart-site

For dependents to work properly (well they way I think it should work) it
must first look in the same directory as the open file, and then it must
walk up the subdirectories looking in each one until arriving at the root
before giving up. Someone might build their file architecture many
subdirectories deep and they thus may import a file a few subdirectories
deep. In my case I organized so any import only goes at most one
subdirectory lower.

Looking "down" a path tree for a dependent is not necessary despite working
in that test I did.

Hope this feedback helps

On Mon, Feb 9, 2015 at 8:45 PM, Joel Kemp [email protected] wrote:

@dkebler https://github.com/dkebler This was fixed in version 2.4.6
(submitted to package_control and pending release within the hour). The
lookup order is detailed here:
https://github.com/mrjoelkemp/sublime-dependents/releases/tag/v2.4.6.

I have to make sure this bug doesn't affect the Find Dependents feature.

Thanks again for contributing.


Reply to this email directly or view it on GitHub
#103 (comment)
.

Cheers, Cuidate, TTFN http://www.youtube.com/watch?v=5Gu50vq5ux4
David

@dkebler
Copy link
Contributor Author

dkebler commented Feb 11, 2015

another thought. If sublime provides a hook to the currently open files
check those as dependents first because they are likely to be open from
someone drilling down through dependencies.

On Tue, Feb 10, 2015 at 9:00 PM, David Kebler [email protected] wrote:

With latest version I can now drill down dependencies multiple
layers(directories) deep. That seems to be working.

But
Dependents ONLY works looking for a dependent at the root from one layer
deep.
Finding dependents within the same subdirectory of the open file fails
(except in root directory)
as well as finding dependent when open file two or more subdirectories
deep.

As an example. Drill down from the sass root, main file of my repo
(framework/scss/sitestyle.scss), to the 'base/base-theme then to 'layouts'
then to 'layout/page' (works), but try going back (dependents) from any of
those files and it will only work from 'base-theme'.

https://github.com/dkebler/hugo-quickstart-site/blob/master/framework/scss/sitestyle.scss

Just for grins I put a 'test' dependency in the /base/layout/_page.scss
file that looks up one directory '../_test.scss'. That works but
surprisingly from that file base/_test.scss it finds the _page.scss
dependent one subdirectory down! This is an usual construction despite
working.

full repo here
https://github.com/dkebler/hugo-quickstart-site

For dependents to work properly (well they way I think it should work) it
must first look in the same directory as the open file, and then it must
walk up the subdirectories looking in each one until arriving at the root
before giving up. Someone might build their file architecture many
subdirectories deep and they thus may import a file a few subdirectories
deep. In my case I organized so any import only goes at most one
subdirectory lower.

Looking "down" a path tree for a dependent is not necessary despite
working in that test I did.

Hope this feedback helps

On Mon, Feb 9, 2015 at 8:45 PM, Joel Kemp [email protected]
wrote:

@dkebler https://github.com/dkebler This was fixed in version 2.4.6
(submitted to package_control and pending release within the hour). The
lookup order is detailed here:
https://github.com/mrjoelkemp/sublime-dependents/releases/tag/v2.4.6.

I have to make sure this bug doesn't affect the Find Dependents feature.

Thanks again for contributing.


Reply to this email directly or view it on GitHub
#103 (comment)
.

Cheers, Cuidate, TTFN http://www.youtube.com/watch?v=5Gu50vq5ux4
David

Cheers, Cuidate, TTFN http://www.youtube.com/watch?v=5Gu50vq5ux4
David

@mrjoelkemp
Copy link
Collaborator

Thanks @dkebler! Your feedback is invaluable and highly appreciated.

Dependents ONLY works looking for a dependent at the root from one layer deep.

Good catch. I need to fix that in node-dependents: dependents/node-dependents#49. There's also an outstanding bug: #42 that might have been fixed for Sass but not JS.

For dependents to work properly (well they way I think it should work) it must first look in the same directory as the open file, and then it must walk up the subdirectories looking in each one until arriving at the root before giving up.

I think this is the best solution for "jump to dependency" as it would be great to navigate anywhere (both inside and outside the root).

The whole purpose of going with user-specified locations was for performance and to avoid recursively looking through lots of directories (as is the case with larger projects).

I'll prototype this idea and see how it really impacts performance.

If sublime provides a hook to the currently open files check those as dependents first because they are likely to be open from someone drilling down through dependencies.

As a heuristic, that makes sense, but it's an incomplete view of the dependency tree and also has a big assumption on the way people use the plugin.

@dkebler
Copy link
Contributor Author

dkebler commented Feb 11, 2015

To avoid confusion between dependents and dependencies I think I'll opt for
parent and child.

In the case of dependent(parent) lookup I assume your code opens and reads
any potential dependent(parent) looking for the corresponding @import back
to dependency/child file from which it was called?

if so then I understand the performance hit of parsing too many files and
thus I think the root directory setting is good as it restricts the search

Settings could take care of someone wanting to look more places and
accepting the hit. In other words being more flexible.
For example looking into a packages directory (bower/compass). Looking at
different path branches. Looking downward in a path.

So my solution is to have these defaults:
00. Dependent (parent) lookup ends with the finding of a single/first
dependent(parent)
0. Look at the currently open files for dependents(parents) first as a way
of preempting an extended search.

  1. Restrict all dependent (parent) searching to the set root or below
  2. Search ONLY upward in the path branch to the root starting with the
    subdirectory of the open file
  3. and going the other way...Dependency (child) searching is disabled.
    Why....
    because the @import must specify the complete path relative to the open
    file for sass compiling anyway,
    exception being when compass is the sass compiler and compass.rb has
    addpaths outside the sass root we set, put still they are known paths).

These defaults would would satisfy me and they make sense in terms of not
allowing one's file architecture to be a spaghetti of dependencies.
My(Hugo's) scheme is a parent can have many children (but with clear
relative path) but a child can only have one parent. Did you see Hugo's
little blurb. One file to rule them all. That's the root parent
(sitestyle.scss in my case). If your plugin nods its head to Hugo's Sass
guidelines then you would satisfying those following best practices which
is a resonable goal in terms of your time and effort.

BUT if you want to write more code....any of these could be overridden to
provide flexibility to a user albeit with warning about potential
performance hits. This addresses your concern about assuming how people
use the plugin.

On Wed, Feb 11, 2015 at 10:19 AM, Joel Kemp [email protected]
wrote:

Thanks @dkebler https://github.com/dkebler! Your feedback is invaluable
and highly appreciated.

Dependents ONLY works looking for a dependent at the root from one layer
deep.

Good catch. I need to fix that in node-dependents:
dependents/node-dependents#49
dependents/node-dependents#49. There's also
an outstanding bug: #42
#42 that might
have been fixed for Sass but not JS.

For dependents to work properly (well they way I think it should work) it
must first look in the same directory as the open file, and then it must
walk up the subdirectories looking in each one until arriving at the root
before giving up.

I think this is the best solution for "jump to dependency" as it would be
great to navigate anywhere (both inside and outside the root).

The whole purpose of going with user-specified locations was for
performance and to avoid recursively looking through lots of directories
(as is the case with larger projects).

I'll prototype this idea and see how it really impacts performance.

If sublime provides a hook to the currently open files check those as
dependents first because they are likely to be open from someone drilling
down through dependencies.

As a heuristic, that makes sense, but it's an incomplete view of the
dependency tree and also has a big assumption on the way people use the
plugin.


Reply to this email directly or view it on GitHub
#103 (comment)
.

Cheers, Cuidate, TTFN http://www.youtube.com/watch?v=5Gu50vq5ux4
David

@mrjoelkemp
Copy link
Collaborator

In the case of dependent(parent) lookup I assume your code opens and reads any potential dependent(parent) looking for the corresponding @import back to dependency/child file from which it was called?

if so then I understand the performance hit of parsing too many files and
thus I think the root directory setting is good as it restricts the search

Yes, exactly.

My(Hugo's) scheme is a parent can have many children (but with clear relative path) but a child can only have one parent.

I'm going to add tests for this in node-dependents. This style is heavily based on relative pathing, which nodejs programs are somewhat close to, but perhaps not as deeply nested. This case should surface some problems with path resolution.

So my solution is to have these defaults:

Thanks for laying out some suggestions.

Ideally, for dependents, I'd like no root settings and when you want to find a file(s), just search for all JS or Sass files in the current project and do a proper path resolution on each file's dependencies to yield an absolute path per dependency. After that, you have an accurate mapping from the dependency tree to the filesystem tree and can return the parents that have the current file's path as a child.

For jump to dependency, I'd like to take the clicked path and perform the file lookup (different algorithm per language).

  • For Sass, check if a file with the (resolved in the case of relative imports) clicked path exists in current dir, or as an underscored version, or in the neighboring directories until the top-level project directory is hit.
  • For JS, resolve the clicked path name. If it's a relative path or a requirejs plugin path, then that's easy. Otherwise, resolve the path's alias (if any), then you have something like foo/bar and need to find a file foo/bar.js somewhere. We could then mimic sass' lookup style: check current directory for foo/bar.js, then check neighboring directories up until we hit the project's top-level directory.
    • If we went top-down instead, then we run the risk of modules resolving to test files. Specifically a module foo/bar could resolve to test/foo/bar.js instead of public/assets/js/foo/bar.js due to whichever of those two are seen first by the lookup engine.

Sorry for the length, but writing it out was pretty helpful. Thanks for the brainstorm.

@dkebler
Copy link
Contributor Author

dkebler commented Feb 12, 2015

If you are able to store a map of all dependencies then clearly this is
best in terms of not repeating a search over and over and the hit that goes
with it.

The map (a json file?) also solves the problem of dependencies whose
relative path is not specified in the @import but like compass are stored
in another file config.rb. This is big cause it means whatever tool you
are using (e.g. codekit, koala..) to access all your dependencies
(packages) you will find them. In the case of compass no need to open that
file config.rb and look for those paths.

Question is when/how to trigger building the map. (any or all of these??)

TRIGGER MAP BUILDING

  1. When it's first installed (using the currently open project)
  2. When plugin settings file is changed
    1 On first plugin request of a session
  3. When sublime loads.
    2.5 When a project loads
  4. When sublime is idle (is it possible to trigger on idle?). This would
    be nice in terms of apparent performance to the user.
  5. Every so often like a cron job
  6. Clearly..when using the map fails (as in a new despondency was added or
    one was changed)

SCOPE OF SEARCH FOR MAP BUILDING.

Yes the best scope is the open root folder/project. In which case maybe
the .deprc is not needed?

This means a map would be created and stored for each folder opened. I
see other plugins maintain a cache. So given that maps may become
corrupted or not needed maybe include a command (from command palette
only?) to clear a/all cached map(s).

On the off chance someone has a library outside the scope of the open root
folder then either you could support in the settings listing all other full
paths to include searching for the map. Or maybe just tell the user they
must add those folders in their sublime project. The plugin would create a
map from all open folders and their subdirectories.

I would still keep that path setting to override using the project
folder(s) root(s) so someone could restrict the scope to whatever they
please.

DISPLAY THE MAP
Now that you have a map a nice feature of the plugin would be on request to
generate a "humans" version something similar to the linux tree command but
output to new tab/file in sublime.

Glad to help out with brainstorming. When it comes to all this map making
folks who follow some best practice file architecture will find the plugin
quick but flexible.

On Wed, Feb 11, 2015 at 8:45 PM, Joel Kemp [email protected] wrote:

In the case of dependent(parent) lookup I assume your code opens and reads
any potential dependent(parent) looking for the corresponding @import
https://github.com/import back to dependency/child file from which it
was called?

if so then I understand the performance hit of parsing too many files and
thus I think the root directory setting is good as it restricts the search

Yes, exactly.

My(Hugo's) scheme is a parent can have many children (but with clear
relative path) but a child can only have one parent.

I'm going to add tests for this in node-dependents. This style is heavily
based on relative pathing, which nodejs programs are somewhat close to, but
perhaps not as deeply nested. This case should surface some problems with
path resolution.

So my solution is to have these defaults:

Thanks for laying out some suggestions.

Ideally, for dependents, I'd like no root settings and when you want to
find a file(s), just search for all JS or Sass files in the current project
and do a proper path resolution on each file's dependencies to yield an
absolute path per dependency. After that, you have an accurate mapping from
the dependency tree to the filesystem tree and can return the parents that
have the current file's path as a child.

For jump to dependency, I'd like to take the clicked path and perform the
file lookup (different algorithm per language).

  • For Sass, check if a file with the (resolved in the case of relative
    imports) clicked path exists in current dir, or as an underscored version,
    or in the neighboring directories until the top-level project directory is
    hit.
  • For JS, resolve the clicked path name. If it's a relative path or a
    requirejs plugin path, then that's easy. Otherwise, resolve the path's
    alias (if any), then you have something like foo/bar and need to find
    a file foo/bar.js somewhere. We could then mimic sass' lookup style:
    check current directory for foo/bar.js, then check neighboring
    directories up until we hit the project's top-level directory.
    • If we went top-down instead, then we run the risk of modules
      resolving to test files. Specifically a module foo/bar could
      resolve to test/foo/bar.js instead of public/assets/js/foo/bar.js
      due to whichever of those two are seen first by the lookup engine.

Sorry for the length, but writing it out was pretty helpful. Thanks for
the brainstorm.


Reply to this email directly or view it on GitHub
#103 (comment)
.

Cheers, Cuidate, TTFN http://www.youtube.com/watch?v=5Gu50vq5ux4
David

@mrjoelkemp
Copy link
Collaborator

I'm thinking of generating the map (if it doesn't exist) on save of a file or when a feature is triggered and no map exists.

If a map does exist and a file is saved, that file's portion of the map would be recomputed likely using: https://github.com/mrjoelkemp/node-dependency-tree. Then, with an updated map, Find Dependents and other tree-based features would only be limited by the map-search algorithm.

I'll have to see the hooks available in the ST api. I know that linters lint on save, so there has to be something there.

I'll see how this approach goes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants