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

Default stringify to true #65

Closed
wants to merge 1 commit into from
Closed

Default stringify to true #65

wants to merge 1 commit into from

Conversation

zepfietje
Copy link

@zepfietje zepfietje commented May 5, 2020

Status

READY

Breaking Changes

NO

Description

Defaulting stringify to true takes away some boilerplate.
Related issue: #60.
Not sure if this should be seen as a breaking change.

Todos

  • Tests
  • Documentation
  • Examples

@zepfietje zepfietje requested a review from felangel as a code owner May 5, 2020 19:45
@codecov-io
Copy link

Codecov Report

Merging #65 into master will decrease coverage by 2.22%.
The diff coverage is 50.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##            master      #65      +/-   ##
===========================================
- Coverage   100.00%   97.77%   -2.23%     
===========================================
  Files            3        3              
  Lines           45       45              
===========================================
- Hits            45       44       -1     
- Misses           0        1       +1     
Impacted Files Coverage Δ
lib/src/equatable_mixin.dart 88.88% <0.00%> (-11.12%) ⬇️
lib/src/equatable.dart 100.00% <100.00%> (ø)

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 29325c1...f96d301. Read the comment docs.

@felangel
Copy link
Owner

@jorgecoca thoughts on this change? I vaguely remember you were against this at one point.

@felangel felangel added the enhancement New feature or request label May 10, 2020
@felangel felangel added this to In progress in equatable May 10, 2020
@jorgecoca
Copy link

@zepfietje First of all, thank you for your contribution!

When I discussed this feature with @felangel a while ago, my recommendation was to not to set stringify as true by default: many classes marked as Equatable might contain user's personal data (usernames, passwords, SSNs, birthdates...), and a very common use case of these is to combine them with flutter_bloc relying on BlocDelegate for automated logs/analytics.

If we introduce a change like this, we potentially risk leaking a lot of information with a "ghost change": yes, there's a new major API version, but people might apply that change blindly and cause a big security hole.

Instead, if we want to control the value of stringify globally, we could have an EquatableDelegate class, or an equatable.yaml (or any other solution..) to control global behavior. This way, the responsibility is on the developer to make this decision, and not on the library.

I hope this makes sense 🤞 Thank you folks!

@zepfietje
Copy link
Author

@jorgecoca good point. I think the main thing is that during development one wants to be able to view all values of a class marked as Equatable, whereas in production certain private values like you mentioned should be hidden.

We should be able to come up with a smart solution along the lines of your suggestions.

Thanks again for your comments! 👌

@felangel
Copy link
Owner

@jorgecoca @zepfietje thoughts on having it default to true in debug mode?

@jorgecoca
Copy link

Wouldn't that assume that you are running Flutter though? How would that work with pure Dart apps/packages? IMO, I'd still prefer to have access to a delegate/config that lets you set it up with your own settings, and leave it up to the developer to make that decision.

@felangel
Copy link
Owner

Wouldn't that assume that you are running Flutter though? How would that work with pure Dart apps/packages? IMO, I'd still prefer to have access to a delegate/config that lets you set it up with your own settings, and leave it up to the developer to make that decision.

Fair enough 👍

@zepfietje
Copy link
Author

A delegate would probably be the best solution. Then developers can check for debug mode themselves.

@felangel
Copy link
Owner

@zepfietje closing this PR in favor of having an alternative solution (using a delegate or global explicit configuration). I will try to work on this this week 👍

@felangel felangel closed this May 27, 2020
@zepfietje zepfietje deleted the default-stringify-true branch May 29, 2020 19:07
@felangel felangel moved this from In progress to Done in equatable Jul 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
equatable
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants