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

Reduce the number of suppressions for internal access #308

Merged
merged 4 commits into from
Feb 28, 2024

Conversation

qwwdfsad
Copy link
Member

@qwwdfsad qwwdfsad commented Oct 3, 2023

While backporting K2 commits, I've spotted one related to internal declarations.

Instead of backporting it, I decided to tide it up in the face of https://youtrack.jetbrains.com/issue/KT-60866/Phase-out-usages-of-SuppressINVISIBLEREFERENCE-INVISIBLEMEMBER-in-libraries

@qwwdfsad qwwdfsad marked this pull request as ready for review October 3, 2023 16:13
override fun serialize(encoder: Encoder, value: DayOfWeek): Unit = impl.serialize(encoder, value)
}
public object DayOfWeekSerializer : KSerializer<DayOfWeek> by createEnumSerializer<DayOfWeek>(
"kotlinx.datetime.serializers.DayOfWeek",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that it is technically a breaking change: it provide FQ name as serial name (as we do and recommend doing in the serialization) and also replaces Month (which is a copy-paste error I believe)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By what principle this FQ name is built? Is it the name of the serialized type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the documentation:

For generated serializers, serial name is equal to the corresponding class's fully-qualified name or, if overridden, SerialName. Custom serializers should provide a unique serial name that identify both the serializable class and the serializer itself, ignoring type arguments, if they are present.

We consider this one as "the default generated serializer" in fact, thus FQN of the serialized type.
I also opened Kotlin/kotlinx.serialization#2581 to make it clear

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"FQN of the serialized type" - ok, then it should be changed as there is no type with this FQN :)
Perhaps to "kotlinx.datetime.DayOfWeek"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed it consistently across the codebase.
It also led to a format change in tests that you can observe in a separate commit.

@ilya-g
Copy link
Member

ilya-g commented Oct 3, 2023

Please exclude InlineOnly annotation removal, there's no problems with it in K2, as I was told in KT-60866

@dkhalanskyjb dkhalanskyjb added the maintenance Non-user-visible tasks label Nov 29, 2023
@ilya-g ilya-g self-assigned this Nov 29, 2023
@qwwdfsad
Copy link
Member Author

qwwdfsad commented Dec 8, 2023

Done

core/common/src/serializers/DateTimeUnitSerializers.kt Outdated Show resolved Hide resolved
override fun serialize(encoder: Encoder, value: DayOfWeek): Unit = impl.serialize(encoder, value)
}
public object DayOfWeekSerializer : KSerializer<DayOfWeek> by createEnumSerializer<DayOfWeek>(
"kotlinx.datetime.serializers.DayOfWeek",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By what principle this FQ name is built? Is it the name of the serialized type?

@qwwdfsad qwwdfsad requested review from ilya-g and removed request for dkhalanskyjb February 27, 2024 10:30
@qwwdfsad
Copy link
Member Author

@ilya-g sorry for the delay, it got lost from my radar. PTAL

@qwwdfsad qwwdfsad force-pushed the remove-internal-suppress branch 2 times, most recently from ac9c388 to e41adc2 Compare February 27, 2024 17:29
@ilya-g ilya-g mentioned this pull request Feb 28, 2024
@qwwdfsad qwwdfsad merged commit f451df4 into master Feb 28, 2024
1 check passed
@qwwdfsad qwwdfsad deleted the remove-internal-suppress branch February 28, 2024 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Non-user-visible tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants