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

feat: deprecate enum val #8

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

brainwo
Copy link
Contributor

@brainwo brainwo commented Aug 27, 2024

Both State.val and Rating.val are unnecessary. Dart already has Enum.index, this commit marks the unnecessary properties to be deprecated in future major version release.

This changes won't affect users of this library.

- add deprecation message
- update `fsrs_base.dart` to use `Enum.index`
@brainwo brainwo changed the title Deprecate enum val feat: deprecate enum val Aug 27, 2024
@brainwo
Copy link
Contributor Author

brainwo commented Aug 29, 2024

@sonbui00 do you have any thoughts on this?
I would argue removing val is important to make the code cleaner, but I'm still not sure whether it should be deprecated first or just remove it entirely. I don't think val would be used anywhere outside of this library internal logic.

If you prefer to keep val, a getter can be a solution:

 enum Rating {
-  again(1),
-  hard(2),
-  good(3),
-  easy(4);
+  again,
+  hard,
+  good,
+  easy;
 
-  const Rating(this.val);
+  const Rating();
 
-  final int val;
+  int get val => index + 1;
 }

@sonbui00
Copy link
Member

sonbui00 commented Aug 29, 2024

@brainwo

If you prefer to keep val, a getter can be a solution:

I think I don't use index because the Python code use the mapping

https://github.com/open-spaced-repetition/py-fsrs/blob/a200a78858bca48e782d6cdf25c8086a3ee290cf/src/fsrs/models.py#L15

I also agree that the enum should not use the val. I added the val to match with logic from Python. Let's me check if the val have any meaning in the process. As I remeber they use that number in somewhere, maybe on optimizer

@brainwo brainwo mentioned this pull request Aug 29, 2024
9 tasks
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.

2 participants