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

Equatable #6

Merged
merged 4 commits into from
Jul 1, 2019
Merged

Equatable #6

merged 4 commits into from
Jul 1, 2019

Conversation

artemnovichkov
Copy link
Contributor

I added missing Equatable conformance for Text class. Related to #2.
There was an idea to calculate a hash for every Text and textStyle change to speed up the equality check. But what if I don't need to compare the texts? We decided to leave the implementation as is, but we need @disabled opinion here.

@codecov-io
Copy link

codecov-io commented Jul 1, 2019

Codecov Report

Merging #6 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #6      +/-   ##
==========================================
+ Coverage   98.74%   98.79%   +0.04%     
==========================================
  Files          16       16              
  Lines         636      662      +26     
==========================================
+ Hits          628      654      +26     
  Misses          8        8
Impacted Files Coverage Δ
Texstyle/TexstyleTests/TextStyleTests.swift 100% <100%> (ø) ⬆️
Texstyle/Texstyle/Text.swift 100% <100%> (ø) ⬆️
Texstyle/TexstyleTests/TextTests.swift 97% <100%> (+0.29%) ⬆️

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 5e13dd9...f25ef88. Read the comment docs.

@stason314
Copy link
Contributor

Im not sure 🤔 but what if hashValue property willlazy for and will calculated inside of it, so when you compere for first time it will calculated and saved. What do you think? 🤔

Copy link
Collaborator

@rosberry rosberry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm completely fine with this. Doing comparisons using hashes won't be trivial anyway, so we can think about it later.

@artemnovichkov artemnovichkov merged commit f25ef88 into master Jul 1, 2019
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.

None yet

5 participants