Added optional eventName parameter

[erickjtorres]

Added an optional eventName parameter in the Lumberdashclient so the developer can specify the name of the event if they would so choose to. This is specifically useful for firebase since you can log multiple events and as a developer, I would like to have the ability to name all my events differently.

Feedback is appreciated

[felangel]

can we update the documentation above to explain the role of eventName?

[felangel]

I think we should update the documentation

[XinfengMa]

I don't think eventName should be optional though because for firebase analytics logEvent name is a required: https://firebase.google.com/docs/reference/android/com/google/firebase/analytics/FirebaseAnalytics.html#logEvent(java.lang.String,%20android.os.Bundle)
also I think parameter message is an optional, it could be part of extras with key "message"

[noahmateen]

Based on Xinfengs feedback, lets make that change

[jorgecoca]

@XinfengMa @noahmateen Since lumberdash is an abstraction over other libraries, we cannot let implementation details of other frameworks, such as firebase, dictate the API of the base client.
Maybe the naming could be improved, but adding the event name as not optional just because we need it for firebase is not the right answer, since other clients, such as sentry, or the simple client don't need it. That's why I think an optional parameter is a better option.

[XinfengMa]

@jorgecoca Just like you mentioned, lumberdash is abstract layer, seems like abstract a required parameter to an optional one is wrong approach. I totally understand if we abstract an optional parameter to a required one, not other way around though. Anyway I don't think this is the place to make the change. Look at the code of FirebaseLumberdash, loggerName is used to log eventName there: https://pub.dev/documentation/firebase_lumberdash/latest/firebase_lumberdash/FirebaseLumberdash/logMessage.html

[codecov]

Codecov Report

Merging #27 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #27   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines          20     20           
=====================================
  Hits           20     20

Impacted Files	Coverage Δ
lib/src/lumberdash.dart	100% <100%> (ø)	⬆️
lib/src/client/simple_client.dart	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 193fd6a...5f6f20b. Read the comment docs.

[erickjtorres]

@jorgecoca Just like you mentioned, lumberdash is abstract layer, seems like abstract a required parameter to an optional one is wrong approach. I totally understand if we abstract an optional parameter to a required one, not other way around though. Anyway I don't think this is the place to make the change. Look at the code of FirebaseLumberdash, loggerName is used to log eventName there: https://pub.dev/documentation/firebase_lumberdash/latest/firebase_lumberdash/FirebaseLumberdash/logMessage.html

@XinfengMa I think the problem is that it's not always required and so we want to keep it optional. It also prevents us from breaking the current API for anyone that is currently using this. In the case of FirebaseLumberdash I'm creating a follow up PR to change the loggerName to being the value it defaults to if eventName is not provided.

[XinfengMa]

@erickjtorres just felt the abstraction is more tuned to Sentry rather Firebase. Don't think it is a good abstraction. logEvent is very common among most of logging frameworks, maybe create a new method? Anyway if everybody else is OK with the current approach, I won't bee the holdout.

[erickjtorres]

@erickjtorres just felt the abstraction is more tuned to Sentry rather Firebase. Don't think it is a good abstraction. logEvent is very common among most of logging frameworks, maybe create a new method? Anyway if everybody else is OK with the current approach, I won't bee the holdout.

I'm okay changing the approach to a proposed one or leaving it as is and coming up with a better approach later. Whatever the team feels comfortable with

[jorgecoca]

Ok, team, here's the approach we are going to take: I am going to close this PR, and let's plan for a V2. I am going to open an issue on Github where we can discuss what the API should be knowing what we know already about all the clients.

On Monday, after the design phase, we can work on making the changes.

Thank you!

[jorgecoca]

Let's continue the conversation in #28