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

Remove invalid pop directives #184

Merged
merged 1 commit into from
Feb 14, 2020

Conversation

willrowe
Copy link
Contributor

@willrowe willrowe commented Feb 14, 2020

pop only accepts true or an integer larger than zero.

The invalid directives break the syntax in pre-release builds of Sublime. The syntax would not even be loaded. This will ensure compatibility going forward.

- `pop` only accepts `true` or a positive integer larger than zero.
@jfcherng
Copy link

jfcherng commented Feb 14, 2020

@Medalink
Copy link
Owner

@willrowe we should target a specific version of sublime (within packages.json) for this change to retain backward compatibility I think. Comments say it was "ignored" before but to be safe I think I rather mark this change as a forward only change. Can you add the specific build numbers with >= in packages?

@jfcherng
Copy link

jfcherng commented Feb 14, 2020

I don't use this package but just wonder what's the point of using/keeping pop: false? It does nothing in ST 3XXX and causes an error/warning in ST 4XXX.

@Medalink
Copy link
Owner

Medalink commented Feb 14, 2020

I don't use this package but just wonder what's the point of using/keeping pop: false? It does nothing in ST 3XXX and causes an error/warning in ST 4XXX.

Are we 100% certain it does nothing in ST3? If we are or can find a document example (or proven test) that this is indeed true I am willing to make the change. However if we are not then it's much more risky to let this flow backwards when it has worked this way for years.

@willrowe
Copy link
Contributor Author

I believe it would be best to fix this for ST3, even though it technically works by being ignored. false has never been a valid value for pop. In the ST3 docs it explicitly says:

  • pop. Pops the current context off the stack. The only accepted value for this key is true.

I defer to you, of course, if you would still like me to add the version constraint.

@Medalink
Copy link
Owner

I believe it would be best to fix this for ST3, even though it technically works by being ignored. false has never been a valid value for pop. In the ST3 docs it explicitly says:

  • pop. Pops the current context off the stack. The only accepted value for this key is true.

I defer to you, of course, if you would still like me to add the version constraint.

Ahh perfect, this is enough to convince me we can safely remove it. Thanks!

@Medalink Medalink merged commit 6dc89c7 into Medalink:master Feb 14, 2020
@Medalink
Copy link
Owner

Thanks for the contributions!

@willrowe
Copy link
Contributor Author

Thanks for taking a look at this @Medalink, really appreciate the package!

@willrowe willrowe deleted the fix-invalid-pop-directives branch February 14, 2020 15:40
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