Swift Next package added. #8843
Conversation
|
@braver hey, could you please take a look at this one, when you'd have a chance |
|
Interesting, I didn't know someone was working on an actually reasonable Outside of that, while I generally agree that taking over unmaintained packages is a good course of action, the replacement does not support ST3, so we'd need to consider this in our decision. This is not a complete review. I just saw this in my notifications and took a quick look. |
You can always incorporate the final commit of the replaced repository in the new repo (just as a dangling commit or branch) and tag that as the “st3-“ version. |
|
@FichteFoll assigning to you for now, I’d like to wait for what you come up with before we do anything else. ST deserves a good Swift syntax in the default package. |
|
I've tested the syntax on a few files I had lying around from 4 years ago and I gotta say it's pretty good. There are a few things that should be adjusted but are not too important because the general experience is there. @yaroslavyaroslav, if you're willing to move this package into the default packages, I'm sure that would be much appreciated. Even more if you're also willing to provide a bit of maintenance in the future as I don't think there are a lot of Swift users among the other contributors (read: probably none). We would then most likely rename the existing package on PC as a preparation to allow for a smooth-ish transition once we get a sign from Sublime Hq that they are indeed willing to merge the syntax (without necessarily passing the review). Anyway, here's some quick feedback on the package as it is now. This could also be part of the review discussion for a PR to the default packages, but that doesn't exist currently. Note that this is also on a fairly detailed level and I'd generally not be so critical, but when we're talking about including it in the default packages, I need to be a bit stricter:
switch states {
case (.off, let foreOut, let foreIn) where foreOut.rawValue > 0 && foreIn.rawValue > 0:
return .takingOff(.heel)
case (let heel, .off, let foreIn) where heel.rawValue > 0 && foreIn.rawValue > 0:
return .takingOff(.foreOut)
case (let heel, let foreOut, .off) where heel.rawValue > 0 && foreOut.rawValue > 0:
return .takingOff(.foreIn)
}
init<T>(from value: T) {
var value = value
self.init(buffer: UnsafeBufferPointer(start: &value, count: 1))
}Finally, as a quick reminder, don't forget to include the license in a PR to the packages repo. |
|
A clone of this package is sitting around in my Setup for months, but never got time to review it seriously. It looks promising on a first glance. Might probably try to make more use of named contexts. I'd expect proper syntax tests to be present as a least pre-condition to make it a default packages candidate. |
@deathaxe, just to clarify, does this contradicts with @FichteFoll request to move this PR to default packages repo, or is it nothing more than an additional task to got it approved? |
|
@FichteFoll Yep, I believe I'd capable to manage that, I mean both, to move this PR to a default package and to resolve some bug fixes during the review process. Although I just like to highlight that language markups isn't somehow related to my main scope of proficiency, so I'd be in my turn greatly appreciated for kind-of-dunno guidance during reviewing process. |
|
No contradiction. Just another task to be completed in order to add it to ST's default packages. Syntax tests are crucial to verify a certain level of quality and to ensure not to break things by future changes. With regards to this PR, we in general try not to add multiple packages with nearly the same function. So if this package was to be added here, it should probably replace existing "Swift" syntax. Maybe by restricting the existing one to ST3 and use the new one for ST4. |
I see a few files in the test dir, is this it or you're talking about something more advanced?
Isn't these are mutually exclusive options with moving this PR to a default packages repo? And in the case they are, which one is preferred one? |
|
Sublime Text supports automated syntax tests to make sure each token receives desired scopes. Those tests are run by CI each time changes are pushed upstream. Example: https://github.com/sublimehq/Packages/blob/master/Java/tests/syntax_test_java.java
Sure. Just expessing expectations that solution, just in case. |
|
I believe I have the strength to undertake the test implementation task, although it might take me a considerable amount of time. At best, I don't see it taking less than a few months. So, from my perspective, I'm completely fine not having this package within Package Control for the duration, as long as I can have it locally on my laptop. Is this acceptable to you @FichteFoll, or should we consider initially taking a shortcut by releasing it here with only a few minor bug fixes as it stands now? As a final question for @deathaxe, I perused the Packages ci/cd config and downloaded |
|
You can run syntax tests directly from within Sublime Text. Just hit ctrl+shift+b to open build system while sublime-syntax file or syntax_test_... file is open and chose "Run Syntax Tests" |
|
@aerobounce FYI Maybe you could comment here on the plans of adding Swift to the default packages? Would you be willing to work on that and/or future support (i.e. updating it yourself or reviewing PRs)? |
I don't really care, but if we intend to only have it on PC temporarily, I also vote against taking over the old Swift package name and go ahead with "Swift Next" or similar, as in the initial proposal. We don't need to disrupt users' workflow more often than necessary. |
Not sure that I got this one. Whether it about to drop it from a PC version afterwards it would be covered with tests or to replace such package later on, when it'll be ready. But anyway I have to say that while messing with this package with my own swift project I found few more bugs and pretty sure more to come. So it definitely worth to me to revise it carefully while covering it with tests. So I consider the task of including this package in the default repo as a mid term at its best now yet still achievable with my own efforts. |
|
I understand. Then I propose the following points of action. Short-term:
Mid-term:
|
|
Sounds like a plan 👍🏻 |
|
It's an explicit way, which involves quite some steps and package renames though. PC4.0 will handle built-in package overrides in sane ways (see: wbond/package_control@3d35ece). They will behave like any other 3rd-party package. If Swift Next is judged as being quite good already, I wonder if there's a more transparent way for end users to move on. Do we need to retain current Swift package for everyone or would it be good enough to move on as we did with other packages such as Less, Liquid, ...
This way:
The downside might be ST4 users not being able to actively choose the old syntax anymore. |
Not easily via package control perhaps, but users can always be pointed to a commit of a given repo that can be installed manually. I've handled breaking changes in packages that way before, it's fine. |
|
We could also do what @deathaxe proposed. I won't really have time for managing any of this for the next 1.5 weeks at least, but that's probably not much of a problem since you should have the necessary permissions as well. It's good to know that PC 4.0 will have some code for handling a situation like this and it's not like ST4 users would lose out on many of the improvements done to Swift in the default package since it'd be basically overriding itself. From a user perspective, I presume this to be way with the least effort required to get the latest Swift syntax definition available to you. |
This reverts commit 7821389.
|
Well it took a while, but here we are. I've fixed not all but most of issues provided in your feedback.
defining decimal point as a separate punctuation sign. I do see that it presented unhighlighted towards the rest of a number, but I do see that this was an intent of the author as well. So I decided to made it nested (i.e.
As you can notice I've skipped Just to clarify the last one, it doesn't mean that I refuse to fix it completely, but it is mean that I won't do it anytime soon. in addition to that I reverted the renaming commit in here, so the Package is called Swift-Next yet again. PS: Just to clarify things up: the main plan for now is to release this boi as a stand alone package. UPD: changes haven't been merged in master and tagged yet, I'm planning to make it happen when I get an approval here. |
|
It works the other way around, you need to update your package including tags and everything for an approval here and before this merge will happen. |
Np, main updated with 1.0.2 release tag. |
|
@FichteFoll @braver |
yaroslavyaroslav
force-pushed
the
swift-next
branch
from
4cb28f5 to
6229f62
Compare
There was a problem hiding this comment.
Automated testing result: SUCCESS
Repo link: Swift Next
Packages added:
- Swift Next
Processing package "Swift Next"
- All checks passed
|
Alright, let's get this package out there! @deathaxe @FichteFoll we can do an iteration later, where we do basically what @deathaxe suggested. For now, let this new package prove itself in the wild for a bit, there is no rush. |
I saw your reply but was just busy with other stuff over the past weeks. Reviewing a package (and especially a huge syntax definition) takes some time, even when you're just iterating over stuff you had previously seen, and I want to give it the attention it deserves instead of doing a rushed analysis, if you could even call it that. That said, we were in agreement over how to handle this package going forward (get it onto PC first, think about making it a default package later) and the first step was just completed, so let's see how the package fares in the near future. I personally do not have any interest in working with Swift any time soon, so I don't have much interest in a hands-on review, but I'm confident we'll eventually manage something when it comes to aligning it more with the default packages eventually, which is an ongoing process even for the syntaxes that we already have.
No, it's quite alright. This is what you should be doing. As for the rest, they are too minor to discuss them in detail here. I'll just add a refence to this package in the latest Swift issue/PR on the default packages and move on for a while until we get back to it. |
This is the ST4 sublime-syntax support for Swift. I must clarify that I am not the author of this package. However, I firmly believe in the importance of this release, mainly due to the subpar quality of the currently available alternatives in the package control.
There are a few of them:
I recently discovered this package quite by chance and was actually stunned by the amount of work that the developer had put into it. Unfortunately, the main repo seems to have been neglected for half a year too. So, I merged all the pending PRs from there, created an organization, and relocated the updated repo there. In this way, I was able to invite the author of this package to the organization, granting him the owner rights (still pending) and all the recognition for this outstanding work.
This PR follows two main objectives: