Skip to content

Conversation

@Thom1729
Copy link
Member

@Thom1729 Thom1729 commented Dec 2, 2018

Closes #67.

A resource's package is significant even if the resource is in Cache, so I think it makes sense to expose the package property for those paths. Then, I don't see any reason to disable it for other paths.

The syntax submodule cares about load order, so I improved the implementation to use ResourcePath.

Copy link
Member

@FichteFoll FichteFoll left a comment

Choose a reason for hiding this comment

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

Using ResourcePath in syntax is very nice. However, this requires a few changes.

@FichteFoll FichteFoll added this to the 1.2.0 milestone Dec 6, 2018
@Thom1729
Copy link
Member Author

Thom1729 commented Dec 6, 2018

Given:

  1. Load order seems to be the natural ordering of resource paths.
  2. Load order is complicated and possibly not fully understood.
  3. Changing the ordering relation would be a breaking change.
  4. Using ResourcePath in syntax is a good thing either way.

I suggest that for the time being we:

  • Pull __lt__ and make ResourcePath not orderable.
  • Preserve the order of sublime.find_resources when returning a list of ResourcePaths rather than explicitly sorting.
  • For children, explicitly document that the order is not guaranteed (because not all children necessarily point to extant resources).

Then, we can add ordering (in 1.2 or another release) when we work out the details and if it won't be a mess. Meanwhile, if a user wants to sort a list of ResourcePaths, they can specify their own sort key (such as str()).

Sound reasonable?

@FichteFoll
Copy link
Member

FichteFoll commented Dec 7, 2018

Yes to all.

We could still implement a function to order resources properly (using cached (installed/default) packages lists), but that should be somewhere else and not in __lt__.

@Thom1729
Copy link
Member Author

Thom1729 commented Dec 7, 2018

Removed explicit comparison and added a function for use in syntax.

Because sublime.load_resources() apparently has no order guarantees, I didn't add anything about ordering to the docs, one way or the other.

Given that we're hopefully expecting a new API method to solve the problem for syntaxes, this should probably do the trick for now.

}


def sort_by_load_order(paths):
Copy link
Member

Choose a reason for hiding this comment

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

Would like to have a docstring here explaining roughly what it does (i.e. to what point it mirrors the internal order).

Case-insensitivity is missing, as far as I can tell.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've implemented case-insensitivity.

We're not exporting the method, so users will only see the comments if they look at the code. At that point, I think that an explicit description of the algorithm would be redundant with the code.

Do we know of any specific cases in which the current logic will fail? If so, then we should definitely fix or document those.

Copy link
Member

Choose a reason for hiding this comment

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

I know it's not exported, but I believe this makes sense to document even for internal use. Most specifically, I wanted to either have case-insensitivity implemented or the fact it's missing documented.

I believe that we hit all known corner cases currently. The easiest way to check would be to verify that sorting a list returned by sublime.find_resources('') does not change it. With a couple extra edge cases added manually, if desired. That could even be unittested, although that is relying on the implementation detail of find_resources returning things in order, until a different API that ensures this is added.

Copy link
Member Author

Choose a reason for hiding this comment

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

After more testing, it looks like the current implementation still sorts some paths differently from sublime.find_resources (e.g. .DS_Store). If sublime.find_resources is the closest we have to a canonical load order, then I may as well just modify syntax to use that ordering directly and remove all of the explicit sorting code. I wish I could be sure that that's absolutely correct, but it might be good enough to tide us over until the new API method obviates the whole thing.

@Thom1729
Copy link
Member Author

Latest commit removes all load order sorting and modifies syntax to use the order of sublime.load_resources.

@FichteFoll FichteFoll merged commit 8415878 into master Dec 10, 2018
@FichteFoll FichteFoll deleted the resource-path-load-order branch January 5, 2019 01:02
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