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.

Sign up for GitHub

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

Jump to bottom

Tweak config and exception #91

Merged
merged 12 commits into from
Jan 15, 2022
Merged

Tweak config and exception #91

merged 12 commits into from
Jan 15, 2022

Conversation

Peanuuutz
Copy link
Contributor

orchestr7
orchestr7 reviewed Jan 11, 2022
View reviewed changes
ktoml-core/src/commonMain/kotlin/com/akuleshov7/ktoml/exceptions/TomlSerializationExceptions.kt Outdated

import kotlinx.serialization.SerializationException

public sealed class TomlDecodingException(message: String) : SerializationException(message)
Copy link
Owner

@orchestr7 orchestr7 Jan 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If you have finally renamed it - may be make all exception's naming consistent? 😄
Let's select one of two variants:

  1. All of them are with 'Toml' prefix
  2. All of them are without a Toml prefix and only the base class TomlDecodingException contains it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of them are without a Toml prefix and only the base class TomlDecodingException contains it

I prefer this, and I imagine something like TomlEncodingException for encoding. I'd like to make both of these as public for users to manipulate, while actual exception types keep internal.

Copy link
Contributor Author

@Peanuuutz Peanuuutz Jan 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

A further question: If there is only one file in exceptions folder, I think to put it out along with Toml and other stuff may be better, as that folder seems redundant.

Copy link
Owner

@orchestr7 orchestr7 Jan 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I prefer this, and I imagine something like TomlEncodingException for encoding

totally agree: TomlEncodingException/TomlDecodingException sounds very good

Copy link
Owner

@orchestr7 orchestr7 Jan 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

A further question: If there is only one file in exceptions folder, I think to put it out along with Toml and other stuff may be better, as that folder seems redundant.

Hm, but that will change package name. Instead of ktoml.exceptions.BlaBlaException - you will have ktoml.BlaBlaException. May be better to have it as is...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, but that will change package name. Instead of ktoml.exceptions.BlaBlaException - you will have ktoml.BlaBlaException. May be better to have it as is...

Then I'll have TomlEncodingException in one file, and TomlDecodingException another one.

@orchestr7
Copy link
Owner

orchestr7 commented Jan 11, 2022 •
edited
Loading

Looks good, but it will completely break all users in the community 😭 😭 😭
Good thing is that we are still in 0 version

Let's wait for #90 being merged...

@Peanuuutz
Copy link
Contributor Author

Peanuuutz commented Jan 12, 2022 •
edited
Loading

Looks good, but it will completely break all users in the community 😭 😭 😭

I have that in mind, so this problem could be solved later. Just put an @Deprecated to inform users about this change. I'll rollback soon and commit a new one like I said.

@Peanuuutz
Copy link
Contributor Author

Done.

  • Marked KtomlConf as @Deprecated, and created TomlConfig in advance(plan to migrate on 0.4.0).
  • Tweak exception classes.
  • Changed field to property because the latter is preferred in Kotlin.

orchestr7
orchestr7 approved these changes Jan 12, 2022
View reviewed changes
Copy link
Owner

@orchestr7 orchestr7 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm, let's merge it after #90
and please fix diktat and detekt issues
If you don't mind - I will add you to this repo as a maintainer, so I won't need to approve CI/CD actions each time

image

@Peanuuutz
Copy link
Contributor Author

diKTat errors this: public value class ... should be value public class ..., which makes no sense to me. 😶

@orchestr7
Copy link
Owner

orchestr7 commented Jan 14, 2022 •
edited
Loading

diKTat errors this: public value class ... should be value public class ..., which makes no sense to me. 😶

created saveourtool/diktat#1170 and assigned on @Cheshiriks. I guess it should be fixed in the nearest future.

@Peanuuutz could you please suppress this case with a @Suppress("WRONG_MULTIPLE_MODIFIERS_ORDER")?

@orchestr7
Copy link
Owner

If possible - could you please add the section Acknowledgement to the readme in this PR and mention something like Thanks to NightEule5 Peanuuutz petertrr Olivki and edrd-f there?

@Peanuuutz
Copy link
Contributor Author

I'm not good at paperwork so feel free to edit. :P

@orchestr7
Copy link
Owner

orchestr7 commented Jan 14, 2022 •
edited
Loading

I'm not good at paperwork so feel free to edit. :P

ok, sure 😄
I just did not want to open a new PR, can I use this one?

@Peanuuutz
Copy link
Contributor Author

Sure 💪

@Peanuuutz Peanuuutz changed the title Tweak config and exception Tweak config, exception and README.md Jan 14, 2022
@orchestr7
Copy link
Owner

orchestr7 commented Jan 14, 2022 •
edited
Loading

Sure 💪

oh, this branch is in fork. Never mind, I will open a new PR :)

@Peanuuutz Peanuuutz changed the title Tweak config, exception and README.md Tweak config and exception Jan 14, 2022
Peanuuutz
Peanuuutz commented Jan 15, 2022
View reviewed changes
Copy link
Contributor Author

@Peanuuutz Peanuuutz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Reviewed again. Ready to merge.

@orchestr7 orchestr7 merged commit cc6e05a into orchestr7:main Jan 15, 2022
@orchestr7
Copy link
Owner

orchestr7 commented Jan 15, 2022 •
edited
Loading

Reviewed again. Ready to merge.

Merged, but as you are a maintainer of this repo, you can merge by yourself next time. But please use squash-merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orchestr7 orchestr7 orchestr7 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Peanuuutz @orchestr7