Revisiting (local) abbreviated variable naming

[BasThomas]

I was talking about this with @jshier and we were of the opinion that changing abbreviations to their complete words can help prevent misunderstandings as well as improve general comprehensibility.

Things like ctx instead of context, for example.

Thoughts?

_{Sent with GitHawk}

[Lukasa]

In general while I’m sympathetic to this, the code churn is high. For stuff in purely local scope we may take the patch, for anything in an API I think we’d prefer not to for now.

[BasThomas]

That sounds reasonable. Any specific reasons or ideas on why you’ve been using abbreviations in the first place?

And would this be something to consider for 2.0? Deprecation is a possibility as well.

[weissi]

now would be the time, what do we think? CC'ing some folks who might have opinions @helje5 / @tanner0101 / @ianpartridge / @pushkarnk / @MrMage / @tachyonics / @airspeedswift / @moiseev / @kjessup / @normanmaurer / @Lukasa / @Joannis

[Lukasa]

As I have said on the ByteBuffer issue, I think the code churn here is pretty rough and the magnitude of the gain is not very high. The downside of this over the change in to ByteBuffer is that we cannot easily use deprecation to ease the transition: we just have to break things. No strong opinions here though.

[MrMage]

I also prefer e.g. context over ctx and would not mind the necessary changes in SwiftGRPC-NIO (but that project is still fairly small anyway). 2.0 sounds like the right time to do this (if done at all), but if this would make eg. git blame unusually due to code churn, that should be considered as well. OTOH, maybe there are other changes in 2.0 that would cause churn anyway, in which case the extra churn introduced by this would be less of an issue?

[pushkarnk]

I'd have wanted the names in the API to be complete words, only because IMO that'd have made the API more "Swifty". Nevertheless, personally speaking, I've gotten used to the abbreviations now and they are good, given that changing them would be quite a big exercise for everyone. For local variable names, I think we should have complete words.

[ktoso]

For what it's worth , personally always try to avoid "ctx" and other shorthands in API unless good rationale for it exists (ctx being a specific pet peeve I've been trying to avoid in libraries).

Though not a big deal I guess, and since it comes with a cost of changing a lot of code – I guess it's mostly a call of if it annoys people enough to warrant the change.

[helje5]

IMO it is fine to do changes like that in 2.0. Won't be compatible anyways. Probably won't take projects more than a day to work through those changes, it is stupid work, but 🤷‍♀️ If the result looks nicer, I'm all + for it.

[tachyonics]

I'd agree with the comments here, its not going to be a lot of work and full words are preferable. For us, we are planning to major version in step with NIO. There would be a concern if a project was going to try to support both NIO 1 and 2 through conditional compilation. That would make a codebase messy and less readable. Is NIO going to have guidance on how to handle that transition from 1 to 2? Changes like this are probably going to push consuming projects towards a clean major version break as well. It might be worth NIO formalizing that into a recommendation for projects on how to handle the version transition.

[weissi]

@tachyonics I think we were hoping to get to a place where we can have 'near automatic' upgrades for NIO 1 codebases to NIO 2. Near automatic as in you run a script and/or apply some fixits and ready to go for most projects. We already promised a document that tells you which changes will be needed and I'd hope it's mostly stuff that could be searched and replaced. I was also thinking (but haven't proposed to the team yet) that for the first version we offer a _NIO1API or something module that would contain extensions/typealiases/etc with @available(*, deprecated, renamed: "nameInNIO2") annotations which would give you the appropriate fix-its. I think it should be an underscored module which just exists for the fixits and isn't part of the public API.

Now changing ctx to context wouldn't work with automatic fix-its but I guess a regex search for \<ctx\> and replace with context should really do the job. Sure it would also replace the user's ctx variables and functions and stuff but you'd think there aren't too many and again.

Or in other words: I don't think we want to invest energy in allowing people to be compatible with both NIO 1 and NIO 2 at the same time but we also don't want to change everything 😉.

[weissi]

Ok, I just gave it a shot: I literally ran

find . -name '*.swift' | while read -r line; do gsed -ri 's/\<ctx\>/context/g' "$line"; done

over the SwiftNIO codebase. It compiled and all tests ran. So we could do this technically... (see #670 for the diff which is obviously large)

[tachyonics]

@weissi that sounds fine. Search and replace changes - as shown - are pretty straight forward and unlikely to cause issues.

Or in other words: I don't think we want to invest energy in allowing people to be compatible with both NIO 1 and NIO 2 at the same time

Absolutely. As long as that is clearly signposted and so far you guys are doing a great job doing so 😀

[AlanQuatermain]

Is there an opportunity to hook into the standard Swift refactoring for this? I imagine users would have a higher degree of confidence in that rather than a “dumb” search & replace of raw text.

FWIW I’d be quite happy to implement a NIO refactoring tool for clients to use; it seems like a nice way to introduce myself to the LLVM tool chain codebase without completely losing my mind in the process 😉

Sent from my iPhone

On Nov 27, 2018, at 10:57 AM, Johannes Weiss notifications@github.com wrote:

Ok, I just gave it a shot: I literally ran

find . -name '*.swift' | while read -r line; do gsed -ri 's/\<ctx>/context/g' "$line"; done over the SwiftNIO codebase. It compiled and all tests ran. So we could do this technically... (see #670 for the diff which is obviously large)

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[weissi]

Is there an opportunity to hook into the standard Swift refactoring for this?

Regarding 'standard Swift refactoring', I'm only aware of the fixits which can be triggered by @available(*, deprecated, renamed: "nameInNIO2"). But that won't work for renaming ctx to context unfortunately. Is there any more tooling I'm unaware of?

I imagine users would have a higher degree of confidence in that rather than a “dumb” search & replace of raw text. FWIW I’d be quite happy to implement a NIO refactoring tool for clients to use; it seems like a nice way to introduce myself to the LLVM tool chain codebase without completely losing my mind in the process 😉

That would obviously be amazing and maybe feasible with SwiftSyntax

[AlanQuatermain]

I was thinking of the stuff that refractors from Swift 2 to Swift 3 syntax, which is run by Xcode but is implemented (I seem to recall) within LLVM.

Sent from my iPhone

On Nov 27, 2018, at 11:19 AM, Johannes Weiss notifications@github.com wrote:

Is there an opportunity to hook into the standard Swift refactoring for this?

Regarding 'standard Swift refactoring', I'm only aware of the fixits which can be triggered by @available(*, deprecated, renamed: "nameInNIO2"). But that won't work for renaming ctx to context unfortunately. Is there any more tooling I'm unaware of?

I imagine users would have a higher degree of confidence in that rather than a “dumb” search & replace of raw text. FWIW I’d be quite happy to implement a NIO refactoring tool for clients to use; it seems like a nice way to introduce myself to the LLVM tool chain codebase without completely losing my mind in the process 😉

That would obviously be amazing and maybe feasible with SwiftSyntax

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[helje5]

I think we were hoping to get to a place where we can have 'near automatic' upgrades for NIO 1 codebases to NIO 2

TBH I think this is/would-be wasted effort. No offence, but I don't think the deployment of NIO (not even Swift itself) is already at a level which requires that kind of effort. Rather focus on getting the API better now, while you still can.

[weissi]

I was thinking of the stuff that refractors from Swift 2 to Swift 3 syntax, which is run by Xcode but is implemented (I seem to recall) within LLVM. …

Oh yes, you're right, there are the Xcode migrators! Let's CC @jrose-apple to ask if that's some kind of engine we could reuse.

[weissi]

I think we were hoping to get to a place where we can have 'near automatic' upgrades for NIO 1 codebases to NIO 2

TBH I think this is/would-be wasted effort. No offence, but I don't think the deployment of NIO (not even Swift itself) is already at a level which requires that kind of effort. Rather focus on getting the API better now, while you still can.

Well, I think some script that does most of the changes automatically would be great and I wouldn't mind writing it (it would be ugly sed rules). But if someone volunteers to write a better tool that's actually not just text replacing that's great even if it's maybe not strictly necessary?

[helje5]

I can only talk for myself, but I'm pretty sure I would not use such a script. I also never ever used the Swift migration stuff in Xcode (and I do this since Swift 1.x). What for? I rather review the changes affecting my code and adjust accordingly. If I need it, I can easily do a global replace in TextMate (no script necessary).

[jrose-apple]

@akyrtzi and @nkcsgexi work on Swift's migrator, but I don't think it's really what you'd want for something like this. SwiftSyntax sounds more like the way to go.

[BasThomas]

Just to chime in as I originally proposed this: I think now is indeed the best time for such a change. And as I see it, providing clearer names can help in the long run while only being "painful" (but with all the thoughts and work on enabling an automatic upgrade, not so much) during migration.

That said, I think it would help to get a list of changes to the API (apart from changing ctx to context) to get a better overview of what the changes are and what the required effort for migrating will be.

[weissi]

@BasThomas I already started a list of renames. Whilst developing the changes, I want it to be a requirement that in the same PR you also add to a list (markdown document) which documents all the changes from NIO 1 to NIO 2.

[normanmaurer]

context works for me

[weissi]

ok, let's make this happen :)