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

⚡ flow: Avoid multiple reinstanciations #149

Merged
merged 7 commits into from
Dec 7, 2017
Merged

⚡ flow: Avoid multiple reinstanciations #149

merged 7 commits into from
Dec 7, 2017

Conversation

nlepage
Copy link
Member

@nlepage nlepage commented Dec 5, 2017

Issue : fix #137

@nlepage nlepage added this to the 1.0.0-RC1 milestone Dec 5, 2017
@nlepage nlepage self-assigned this Dec 5, 2017
@codecov-io
Copy link

codecov-io commented Dec 5, 2017

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #149   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          77     77           
  Lines         280    294   +14     
=====================================
+ Hits          280    294   +14
Impacted Files Coverage Δ
packages/immutadot/src/core/apply.js 100% <100%> (ø) ⬆️
packages/immutadot/src/core/path.utils.js 100% <100%> (ø) ⬆️
packages/immutadot/src/flow/flow.js 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 79d0b59...e7d7933. Read the comment docs.

@nlepage
Copy link
Member Author

nlepage commented Dec 6, 2017

For now "already applied paths" are theoric.
For paths containing slices this means that we don't really know if copies were made, which indexes of slices were copied, the entire structure might not have been copied...
That's why path containing slices are filtered from already applied paths...

@frinyvonnick What do you think we should do ?

  • Merge this as it is (with docs, tests, and possibly refactoring) and try to improve in the next version
  • Not merge this
  • Refactor apply to have really applied paths

@frinyvonnick
Copy link
Contributor

I would opt for the first proposal and we open an issue about a future refatoring 👍

Copy link
Contributor

@frinyvonnick frinyvonnick left a comment

Choose a reason for hiding this comment

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

👏

@frinyvonnick frinyvonnick changed the title 🚧⚡ flow: Avoid multiple reinstanciations ⚡ flow: Avoid multiple reinstanciations Dec 7, 2017
@nlepage
Copy link
Member Author

nlepage commented Dec 7, 2017

See #153 for further improvements of apply

@nlepage nlepage merged commit c15e0cc into master Dec 7, 2017
@nlepage nlepage deleted the enhance/137 branch December 7, 2017 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flow: Avoid multiple reinstanciations
3 participants