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

Moves Files adapters to external packages #1172

Merged
merged 3 commits into from
Mar 24, 2016

Conversation

flovilmart
Copy link
Contributor

No description provided.

@flovilmart flovilmart force-pushed the flovilmart.externalAdapters branch from 00f2aac to 1e45ac7 Compare March 24, 2016 04:48
@facebook-github-bot
Copy link

@flovilmart updated the pull request.

- Something weird is happening here, instanceof is not giving the right value
@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@codecov-io
Copy link

Current coverage is 92.52%

Merging #1172 into master will increase coverage by +0.65% as of 29bc535

@@            master   #1172   diff @@
======================================
  Files           93      90     -3
  Stmts         5812    5633   -179
  Branches      1056    1015    -41
  Methods          0       0       
======================================
- Hit           5340    5212   -128
  Partial         11      11       
+ Missed         461     410    -51

Review entire Coverage Diff as of 29bc535

Powered by Codecov. Updated on successful CI builds.

*
*
*
*
* @param config the current config
Copy link
Contributor

Choose a reason for hiding this comment

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

Update comments

@drew-gross
Copy link
Contributor

Looks awesome to me but I guess because of removal of config parameter it's a breaking change for any other file adapters (are there any?) so I would like a second opinion @gfosco @nlutsenko

@nlutsenko
Copy link
Contributor

It definitely is, but looks like a good change. We would need to increment a minor version for the next release, hence breaking changes. I wish we moved GridStoreAdapter as well...

@drew-gross
Copy link
Contributor

Lets add some info about the breaking change and how to deal with it in the changelog or release notes or something, then merge.

@facebook-github-bot
Copy link

@flovilmart updated the pull request.

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.

5 participants