search

wbond / package_control

The Sublime Text package manager
https://packagecontrol.io
4.79k stars 814 forks source link

Update scheme to 4.0.0 #1527

Closed deathaxe closed 3 years ago

deathaxe commented 3 years ago

Fixes #1516

This PR bumps repository and channel scheme version to 4.0.0

  1. Scheme versions are validated using SemVer class to be able to access major, minor, patch directly.
  2. Json keys and related methods are renamed to use libraries instead of dependencies.
  3. Library releases must provide python_versions: ['3.3', ...] as of scheme version 4.0.0.
  4. Libraries must not provide load_order as of scheme version 4.0.0.
  5. Included repositories' scheme version is validated and must match the scheme version of the including repository. That's required as several scheme changes are handled by checking the root repository's scheme version only.

Notes:

deathaxe commented 3 years ago

The methods in general are quite large and should probably be splitted in order to get some testable pieces, but I don't want to refactor everything just for nothing at this point.

The try blocks are not that large, IMHO. It is exactly the use case of try...catch to wrap algorithms of one or limited purpose which needs to finish en block or fail early without the need to write deeply nested if..else or goto statements. The only reason for the size of the block is the number of objects handled explicitly. All operations are of the same purpose - validating content. So I don't see any issues. IMHO, it's even more consistent this way than trying to continue nested loops and wait to abort execution until the loop is done.

The try catch block could easily be reduced to one line by moving the code to another method, if it is a visual problem.

FichteFoll commented 3 years ago

The PR is fine to me as-is. I mostly asked a question out of curiosity. Of course, splitting would be nice, but not necessarily within the scope of this PR.

wbond commented 3 years ago

I just rebased this onto latest four-point-oh to get the CI working. Hoping to review in the next few days.

Thanks for all of the improvements and cleanup you've done in these PRs @deathaxe!!

wbond commented 3 years ago

Based on feedback related to mdpopups and people putting package resources in dependencies, I propose:

  1. We use schema version 4.0.0
  2. Current dependencies will be renamed to “libraries” and load_order will be dropped.

This will open up the term “dependency” for someone to implement inter-package dependencies. That will benefit packages that want to do things like mdpopups, and make installing things like LSP and SublimeLinter easier. I’m not proposing we need to implement the new dependencies now, just prepare the schema.

deathaxe commented 3 years ago

This also means to rename dependencies.json and all dependencies keys in package metadata and dependency related methods, while keeping backward compatibility with older schemes which still use the old term? Sounds quite invasive change.

wbond commented 3 years ago

Yeah, it does. I’m not expecting you to do all of that, though. More interested in discussion.

Either way all of these dependency changes are going to be pretty invasive and require a bunch of beta testing.

deathaxe commented 3 years ago

  1. Basically the term libraries seems more precise with regards how they are meant to be used in ST. Python doesn't distinguish packages and libraries though.

  2. I don't really see a use case for re-using dependencies in the future.

    1. A today's dependency is a pure python package which will be installed into Lib/.
    2. Packages may depend on each other in the future, so backend-service packages may be pulled automatically when installing others which need them. If those shouldn't be installed manually just a flag might be needed to hide them from quick panels, that's it. I don't see a reason for another kind of package, to be honest.

  3. It's all quite much effort just because of two dependency packages abusing the intended structure. I know @facelessuser is aware of required changes to keep mdpopups working. The only resources lsp_utils provides is a default settings file with a single item and the related command to open the file. Complains about loosing that are exagurated, IMHO. Those settings should be part of the LSP-xxx helper packages as if lsp_utils wouldn't exist.

  4. RepositoryProvider will need some changes as scheme version needs to be validated before reading anything from it. Also found included repositories not beeing validated at all, atm. They are just fetched and merged to repo_info. We should probably make sure included repositories use the same scheme version to prevent bad things from happening. Other than that I'd try to translate the dependencies key from older schemes into libraries in fetch() to avoid too much code branches.

facelessuser commented 3 years ago

Just to note, mdpopups will most likely move to vendoring what it needs in the future. It's just kind of a pain managing that chain, at least for that package. I'd like it to be less sensitive to the libraries updating that it depends on as well. While many of the things it depends on are useful outside of mdpopups, and fine to be standalone dependencies/libraries in ST, I find being able to lock mdpopups to specific, vendored versions will be easier to manage.

deathaxe commented 3 years ago

Ok, renamed a couple of things to move towards libraries. Installing/removing packages/deps still seems to work. Can't run integration tests atm. due to server errors.

wbond commented 3 years ago

I've:

wbond commented 3 years ago

I should note that I did a force push