search

rknell / flutterEnumsToString

Better conversion of ENUMs to string for Flutter / Dart
Other
85 stars 23 forks source link

throws an exception when parsing a non enum item #19

Closed lironhl closed 4 years ago

lironhl commented 4 years ago

Fixes #18.

Throws a NotAnEnumException whenever EnumToString.parse receives a non enum item. Also added tests for this behavior.

Let me know if you want me to change wording, conventions, names or code.

rknell commented 4 years ago

Hey thanks for the PR. I can see you have tried a few times to pass the CI pipeline and put some hints on the README.md (you are not the only one!)

If you can clean that up I'll accept the PR.

lironhl commented 4 years ago

@rknell Done.

rknell commented 4 years ago

Sorry for the delay, PR approved

rknell commented 4 years ago

Hey just heads up I had to roll back the changes from the PR - not forever, but because it turns out the way of checking if its an enum item breaks in release mode for the web (with some reports of release mode for mobile as well)

https://github.com/flutter/flutter/issues/66236

Will roll it back in once that flutter bug is resolved.

mraleph commented 4 years ago

Will roll it back in once that flutter bug is resolved.

Note: that Flutter bug is not going to be resolved, because there is no bug. In general you should not expect that Type.toString returns anything useful (or portable, or stable) - it's there just for debugging purposes essentially.

rydmike commented 4 years ago

Thanks for mentioning this here too @mraleph I was going to bring it up as well.

Using codegen was mentioned by a Flutter team member elsewhere as a potential solution, sounds a bit like overkill for this small package, but probably doable if you want to take that route.

Alternatively maybe it could be enough and possible to just use Assert instead for the type check. Then it would only get used in debug mode where this kind of type check works and warn (bomb) devs about wrong type usage during development and testing, but just not get included at all in release builds. Not really the same thing, but it might be enough as an alternative workaround and offer enough needed robustness for devs during dev phase, even if it does not get included in the release.

Maybe there are some other better ways to approach this as well, or just leave it as it is now.

rknell commented 4 years ago

I just wrapped the call in an assertion and cleaned up the other bits of code. Looks to be working now so have published a new version with the fix 1.0.13. Thanks so much for looking into all this and your guidance @rydmike and @mraleph

I certainly didn't expect code that was working in debug to suddenly not work in release, and it does raise an alarm about any other things that might break as I just wouldn't be aware of them. I certainly think that it would be great to try and wrap some alerts / linting around use of potentially unsafe code as suggested by @rydmike if possible. I certainly don't have any experience with compilers, and would expect many other who are in the dart ecosystem now are in the same boat.

Once again though I really appreciate all the commitment put in to see this through and help out.