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

Upgrade embedded Python to syntax v2 #13

Merged
merged 3 commits into from
Apr 26, 2023
Merged

Upgrade embedded Python to syntax v2 #13

merged 3 commits into from
Apr 26, 2023

Conversation

deathaxe
Copy link
Contributor

@deathaxe deathaxe commented Apr 1, 2023

This PR fixes a compatibility error with ST 4148, which ships with a sublime-syntax v2 Python.

Warning

This fix breaks compatibility with older ST builds. Thus a new release branch at packagecontrol.io is required.

To create a new release branch, I'd recommend to prefix your tags with the least required ST build number.

Example:

  • 4075-1.1.2 (with v1 python)
  • 4148-1.1.2 (with v2 python)

A corresponding change in package_control_channel would be required.

This commit fixes a compatibility error with ST 4148, which ships with
a sublime-syntax v2 Python.

Note:

This fix breaks compatibility with older ST builds. Thus a new release
branch is required.
@nk9
Copy link
Owner

nk9 commented Apr 18, 2023

Thanks for the PR, and for alerting me to this change. I have tried duplicating the Python file (keeping the current v1) and creating a second Python (for Just).sublime-syntax one as in your changes. Having both files seems to work for both versions. Can you confirm that this is an acceptable solution? I anticipate that 1.1.2 would be the last version to support 4075, so I guess I could just remove the v1 file after the 1.1.2 release.

Also, there's this in the embedding file. I presume this still hasn't landed? Is there any problem with leaving the code uncommented?

  string-prototype:
    # Note: not yet supported by ST 4143/4146 (it's in preparation)
    - meta_prepend: true
    - include: scope:source.just#recipe-content-string-interpolations

@deathaxe
Copy link
Contributor Author

Having 2 python syntaxes in the package at the same time doesn't make much sense as only one of them will ever be used.

The strategy how to go on depends on whether future changes/improvements of Just syntax should still be compatible with ST4075 - 4147.

strategy 1 - linear

You could restrict current release to 4075 - 4147 and leave it behind untouched. As ST up to 4147 still ships the v1 python syntax, everything is fine as it is - no vendoring python syntax needed.

A next release with this PR merged would need to be restricted to 4148+ (e.g. by using tag prefixes such as 4148-) and updateing package_control_channel accordingly.

This way Users with older builds (before 4148) wouldn't receive further updates to Just syntax.

strategy 2 - branching off

You could create a st4148 branch with this PR merged. Primary changes to Just would go on in master. With each release master can be merged into st4148 branch.

In master branch a tag with 4075-1.x.x is created and after merging a 4148-1.x.x tag is created in st4148 branch.

This way you could still maintain Just for older ST builds while releasing 2 different variants depending on ST build.

I do it this way with https://github.com/SublimeText/Astro and various other syntax packages.


In neither case vendoring is needed.

Vendoring current v1 python syntax would be required only, if you don't want to re-use v2 syntax of 4148+, but I wouldn't recommend that.


I presume this still hasn't landed? Is there any problem with leaving the code uncommented?

It has landed in ST4148+, but not for those who are still using latest stable release (ST4143).

This context being present or not is not of any problem. It wouldn't just have any effect if base syntax doesn't know about it. If you vendor v1 python it doesn't make sense to keep it, but I don't have enough context to give any advice about it. (Either vendor v1 python and forget this PR, or use this PR with one of strategy 1 or 2).

@nk9
Copy link
Owner

nk9 commented Apr 19, 2023

The more I think about this, the more I'm leaning toward just leaving both versions of the embedded Python syntax in place permanently. Based on your suggestions, it looks like if I'm generating tagged builds with only the single correct embedded Python syntax version, then my options are:

  1. Leave pre-4148 builds marooned at 1.1.1 and continue creating just a single tag for each release (and yet still forever have to prefix my build tags); or
  2. As you suggest, create a long-running branch and merge in changes before tagging. And then create two prefixed tags per release forevermore.

You're right that only one embedded syntax will ever be used, but the way I see it, that's actually perfect. It's analagous to a universal binary where all the pieces are available for 2+ architectures. In this case, it's just two mutually-exclusive versions of the syntax. ST will figure out which one it needs based on what it supports, and I don't have to fork anything or do any version/tag work at all. I even avoid the package control PR. And the only cost is a single, tiny file kept around to support older versions.

My limited testing seems to show that this works, but I want to make sure I'm not misunderstanding something fundamental here. Other than strict code cleanliness, is there a reason not to include both embedded syntax versions?

Thanks very much for your help on this!

@deathaxe
Copy link
Contributor Author

I wouldn't recommend adding both python versions.

In best case the v2 syntax is just ignored and Just only uses vendored v1 syntax on all builts. So the v2 would be of no use and wouldn't need to be present.

In worst case both being present, using same main scope, could cause conflicts or maybe other unexpected weird effects.

If you don't want to create tagged releases, just vendor v1 python and go with it and don't benefit from any future improvements of core python syntax.

@nk9
Copy link
Owner

nk9 commented Apr 19, 2023

Hmm, interesting. When I add a change to only the embedded v1 syntax, but leave both syntax files in place, the change is only shown in 4143. Doesn't this mean that 4148 is ignoring the v1 syntax and only using the v2 syntax? I don't mean to argue with you! Just trying to understand what I'm seeing.

ST 4148 DOES NOT show the change made to v1 syntax

ST 4148 DOES NOT show the change made to v1 syntax

@deathaxe
Copy link
Contributor Author

It "works" because both syntaxes are inherited from same base syntax while only one compiles successfully. Nevertheless console shows an error message for the failed one in both builds 4143 and 4148.

error parsing lexer: Packages/Python/Python.sublime-syntax: syntax inheritance does not allow for mixed versions at line 3 column 1

Exactly this message was, what triggered this PR, while doing research for (sublimehq/Packages#3692).

So in this very special case, having both syntaxes in place wouldn't cause any harm, beyound the error message and ST failing to compile a syntax, but in general I still wouldn't recommend it...

a) to avoid that misleading error message,
b) don't create a precedent and
c) having two syntaxes with same scope may still cause unexpected results if both compile.

@nk9
Copy link
Owner

nk9 commented Apr 24, 2023

I have created the 4075 branch and pushed the latest release with the new tag format for older (stable channel) ST releases.

I am now going to create the package_control_channel PR. Once merged, I'll create the 4148-1.1.2 release.

@nk9
Copy link
Owner

nk9 commented Apr 26, 2023

I've completed this. Thank you for alerting me to the necessary changes.

@nk9 nk9 closed this Apr 26, 2023
@nk9 nk9 reopened this Apr 26, 2023
@nk9
Copy link
Owner

nk9 commented Apr 26, 2023

One test failure to resolve (#16) before this can be closed and the 4148+ build released.

@nk9 nk9 merged commit 439d101 into nk9:main Apr 26, 2023
@deathaxe deathaxe deleted the pr/fix-st4148-compat branch April 27, 2023 15:36
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.

2 participants