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

replace data class with class to fix issue #21 #22

Closed
wants to merge 1 commit into from

Conversation

kazemcodes
Copy link

@kazemcodes kazemcodes commented Aug 22, 2023

after applying code to my own project everything works fine, and there was no issue, and everything should work like before without any migrations.
if possible please release a new update for lyricist, as I want to migrate my project to lyricist.
@DevSrSouza @adrielcafe

@kazemcodes
Copy link
Author

when will this be reviewed?
@DevSrSouza @adrielcafe

Copy link
Collaborator

@DevNatan DevNatan left a comment

Choose a reason for hiding this comment

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

Thx for your contribution @kazemcodes!!

Maybe non-immutable class and fields can lead into something but we will see it later.

Apparently this modification is intended for a small number of Lyricist users, very specific, so can you

  • Place it under a flag like lyricist.useRegularClassToPreventOverflow (as clear as possible) so that it can be defined by user choice. See LyricistConfig as reference
  • Revert SDK version changes as it is not in the scope of this PR

@adrielcafe
Copy link
Owner

Since this change only affects the xml processor (which should be triggered only once for migration purposes) I've done a similar implementation but using interface instead.

Thanks for your bug report!

@adrielcafe adrielcafe closed this Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants