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

Rename classes to follow Kotlin's naming conventions #262

Open
wants to merge 12 commits into
base: dev
from

Conversation

Projects
None yet
4 participants
@BenWoodworth

BenWoodworth commented Nov 4, 2018

This pull request renames some classes to follow Kotlin's naming conventions. Namely:

  • JSON -> Json
  • CBOR -> Cbor

https://kotlinlang.org/docs/reference/coding-conventions.html#naming-rules

When using an acronym as part of a declaration name, capitalize it if it consists of two letters (IOStream); capitalize only the first letter if it is longer (XmlFormatter, HttpInputStream).

@pdvrieze

This comment has been minimized.

Contributor

pdvrieze commented Nov 5, 2018

You should probably add corresponding (deprecated) typealiases to allow for migration. While name changes can still happen in the experimental library a sane migration for a while is good.

BenWoodworth added some commits Nov 5, 2018

Added aliases for renamed classes/functions.
TODO: Type aliases for inner classes.
- Json.JSONInput
- Json.JSONOutput
- Cbor.CBORDecoder
- Cbor.CBOREncoder
Removed unnecessary rename aliases.
TODO: Type aliases for inner classes.
- Cbor.CBORDecoder
- Cbor.CBOREncoder
@BenWoodworth

This comment has been minimized.

BenWoodworth commented Nov 5, 2018

That's a good idea. The only aliases I didn't add yet are for Cbor.CBORDecoder and Cbor.CBOREncoder, since typealiases can't be nested.

@pdvrieze

This comment has been minimized.

Contributor

pdvrieze commented Nov 5, 2018

Don't use hidden deprecation level. Let users gradually migrate. Hidden should be used after a number of releases as it provides only binary compatibility but not source compatibility.

@BenWoodworth

This comment has been minimized.

BenWoodworth commented Nov 5, 2018

They should've all been changed in abba226 (unless I missed some)

@sandwwraith sandwwraith self-requested a review Nov 5, 2018

@sandwwraith

This comment has been minimized.

Collaborator

sandwwraith commented Nov 5, 2018

There's a Deprecated.kt file in a common module, you could use it as a sample.

@sandwwraith

This comment has been minimized.

Collaborator

sandwwraith commented Nov 9, 2018

It's nice now! However, merging it directly to master can create problems for folks which will be trying to use examples and documentation (since version with new names is obviously not published yet, docs in master should use old names). Do you mind opening this PR against dev so I can merge it there?

@BenWoodworth BenWoodworth changed the base branch from master to dev Nov 9, 2018

@BenWoodworth

This comment has been minimized.

BenWoodworth commented Nov 9, 2018

Alright! I changed the base branch of this pull request to dev, so besides some merge conflicts, it should be good to go.

Merge remote-tracking branch 'kotlin/dev'
# Conflicts:
#	runtime/common/src/main/kotlin/kotlinx/serialization/cbor/Cbor.kt
#	runtime/common/src/test/kotlin/kotlinx/serialization/features/SkipDefaults.kt
#	runtime/jvm/src/test/kotlin/kotlinx/serialization/features/GenericCustomSerializerTest.kt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment