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.

Sign up for GitHub

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

Jump to bottom

Improves loading of Push Adapter, fix loading of S3Adapter #833

Merged
merged 2 commits into from
Mar 6, 2016
Merged

Improves loading of Push Adapter, fix loading of S3Adapter #833

merged 2 commits into from
Mar 6, 2016

Conversation

flovilmart
Copy link
Contributor

  • Adds environment variables to configure S3Adapter

Fixes #820
Fixes #832

@drew-gross
Copy link
Contributor

Can you also update the docs to describe the new options?

@flovilmart
Copy link
Contributor Author

Sure!

@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@drew-gross drew-gross closed this Mar 4, 2016
@drew-gross drew-gross reopened this Mar 4, 2016
@codecov-io
Copy link

Current coverage is 91.66%

Merging #833 into master will decrease coverage by -0.02% as of 80f99d5

@@            master    #833   diff @@
======================================
  Files           71      71       
  Stmts         4100    4103     +3
  Branches       843     845     +2
  Methods          0       0       
======================================
+ Hit           3759    3761     +2
  Partial         10      10       
- Missed         331     332     +1

Review entire Coverage Diff as of 80f99d5

Powered by Codecov. Updated on successful CI builds.

@flovilmart flovilmart force-pushed the flovilmart.S3Environment branch from 2fbcd1b to 764c563 Compare March 4, 2016 18:36
@facebook-github-bot
Copy link

@flovilmart updated the pull request.

asciimike
asciimike reviewed Mar 4, 2016
View reviewed changes
src/Adapters/AdapterLoader.js
// incompatible
try {
return loadAdapter(defaultAdapter, undefined, adapter);
} catch (e) {};
}
// return the adapter as is as it's unusable otherwise
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment still correct, or will it just return the object representing the adapter (which is presumably usable)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah? That retuns the adapter as not other loading method seems to be available.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh man, I totally misread that, somehow turned that into "return the adapter as it's unusable". Might I suggest "return the adapter as provided" or something similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@asciimike asciimike mentioned this pull request Mar 4, 2016
Merged
@flovilmart flovilmart force-pushed the flovilmart.S3Environment branch from 764c563 to a44b1d9 Compare March 4, 2016 23:09
@facebook-github-bot
Copy link

@flovilmart updated the pull request.

flovilmart added a commit that referenced this pull request Mar 6, 2016
@flovilmart
Merge pull request #833 from ParsePlatform/flovilmart.S3Environment
375e701 
Improves loading of Push Adapter, fix loading of S3Adapter
@flovilmart flovilmart merged commit 375e701 into master Mar 6, 2016
@flovilmart flovilmart deleted the flovilmart.S3Environment branch March 6, 2016 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@flovilmart flovilmart

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@flovilmart @drew-gross @facebook-github-bot @codecov-io @asciimike @gfosco