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

use enum for EventType or assert them inside methods #40

Open
m1-nann opened this issue Jan 5, 2019 · 5 comments
Open

use enum for EventType or assert them inside methods #40

m1-nann opened this issue Jan 5, 2019 · 5 comments

Comments

@m1-nann
Copy link

m1-nann commented Jan 5, 2019

Since we are using Dart, could we use enum instead of raw String? It's less error-prone, help avoiding typo
it would be something like this

ref.on(EventType.child_added, (snap) => {
  ///do something
})

Or at least we could assert if eventType is valid inside the methods

@pulyaevskiy
Copy link
Owner

I think enum fits well in this case. Only thing is that we should name values without underscores to comply with Dart style guide, e.g. EventType.childAdded.

Note that it would be a breaking change so would have to be released as 2.0.0 which for such a small change might be hard to justify major version bump.

We can approach this in two steps:

  1. Add assertions as per your second suggestions, this would be non breaking. We can also add an abstract class with constants so that users are not forced to hard code strings. E.g.:
abstract class EventType {
  static const String childAdded = 'child_added';
  // ... etc
}

Would be nice to update examples and dartdocs to refer to these constants so that it's easier to discover.

All of this is non-breaking and we can release it in 1.x.x.

  1. When we gather enough breaking changes or there is upstream (JS SDK) change that requires us to bump major version to 2.0.0 we can replace the abstract class with an enum and update all method signatures accordingly.

How does this sound?

@m1-nann
Copy link
Author

m1-nann commented Jan 5, 2019

naming: if we are not using the firebase string name child_added, I suggest considering uppercase low dash (ie. CHILD_ADDED). Maybe it's just a personal preference, though it's also used for firebase.database.ServerValues.TIMESTAMP.

I understand the possible breaking changes, so the two steps are pretty good ideas.

@pulyaevskiy
Copy link
Owner

Here is portion of official Dart style guide re: naming constants and enum values:
https://www.dartlang.org/guides/language/effective-dart/style#prefer-using-lowercamelcase-for-constant-names :

In new code, use lowerCamelCase for constant variables, including enum values.

I'd prefer to stick to the style guide as it makes this Dart code consistent with most of the Dart code available on Pub and the Dart SDK itself.

@m1-nann
Copy link
Author

m1-nann commented Jan 6, 2019

Oh, I see. I was searching for 'constant' and 'enum' but didn't catch it somehow. My bad.

@m1-nann m1-nann closed this as completed Jan 6, 2019
@m1-nann m1-nann reopened this Jan 6, 2019
@pulyaevskiy
Copy link
Owner

FYI: I added EventType with constants as per discussion and released together with your change of the on method as 1.2.0.

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

No branches or pull requests

2 participants