Skip to content

Conversation

clintonb
Copy link
Contributor

  • Inheriting directly from Slumber to avoid having to update the client anytime an endpoint is added/removed.
  • Using custom exceptions specific to this client.

@jimabramson This is in response to your comment about exceptions. FYI @rlucioni

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling eb65fd3 on redux into * on master*.

@clintonb
Copy link
Contributor Author

Note that the client now becomes quite thin/dumb.

@clintonb
Copy link
Contributor Author

@benpatterson can you look into the multiple Travis builds issue for this repo?

@clintonb
Copy link
Contributor Author

@benpatterson also, please disable coveralls comments.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 616969b on redux into * on master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 39efb48 on redux into * on master*.

@rlucioni
Copy link

rlucioni commented May 1, 2015

@clintonb you can address the multiple Travis builds issue by adding the following to this repo's .travis.yml:

branches:
    only:
        - master

For an example, refer to edx/ecommerce's Travis configuration.

@jimabramson
Copy link

@clintonb nicely done. However, this is a more drastic change that I'd envisioned when commenting. I do see the beauty of no longer needing to maintain the client to get access to server methods. But - don't kill me - if we switch over to having developers explicitly interact with slumber's REST api, then rewriting slumber exceptions seems more confusing than helpful. Of course, if the exception-rewriting part were removed there'd be almost nothing left in this client except a wrapper around API.init, which makes me curious what the main intent for separating out the client was.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 4ea334d on redux into * on master*.

@clintonb
Copy link
Contributor Author

clintonb commented May 1, 2015

@jimabramson The biggest benefit of the wrapper is removing the need for consumers to understand JWT. The client definitely needs to be separated from LMS so that it can be consumed by both LMS and our acceptance tests. I can go either way on the exceptions.

@jimabramson
Copy link

@clintonb thanks, makes sense - in that case I think I would be in favor
of just losing the exceptions stuff.

On Friday, May 1, 2015, Clinton Blackburn notifications@github.com wrote:

@jimabramson https://github.com/jimabramson The biggest benefit of the
wrapper is removing the need for consumers to understand JWT. The client
definitely needs to be separated from LMS so that it can be consumed by
both LMS and our acceptance tests. I can go either way on the exceptions.


Reply to this email directly or view it on GitHub
#2 (comment)
.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 54cd879 on redux into * on master*.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jimabramson Any opposition to the imports here? Consumers are slightly cleaner since they don't have to import slumber directly.

Choose a reason for hiding this comment

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

that seems good, yeah.

@jimabramson
Copy link

👍

- Inheriting directly from Slumber to avoid having to update the client anytime an endpoint is added/removed.
- Using custom exceptions specific to this client.
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 3e0fe44 on redux into * on master*.

@rlucioni
Copy link

rlucioni commented May 1, 2015

👍

clintonb added a commit that referenced this pull request May 1, 2015
Reconfigured the client
@clintonb clintonb merged commit 423f634 into master May 1, 2015
@clintonb clintonb deleted the redux branch May 1, 2015 19:50
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.

4 participants