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

[Proposal] Unified Client interface #56

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

suda
Copy link
Contributor

@suda suda commented Feb 10, 2017

Adds build targets support to Client class and drafts a proposal for unified endpoint/resource handling.

Notes about some decisions:

  • model properties are described in comment rather than be initialized in a constructor to allow undefined values but still be documented
  • build_target endpoint is an interesting example because it groups targets by version name but we want to access each build target separately (which would be possible in endpoint V2). So we're normalizing the data and providing the interface which will remain constant even after switching the backing endpoint
  • this PR also tries to propose Error building/handling pattern that combines our three implementations mentioned in the ClubHouse story

@coveralls
Copy link

coveralls commented Feb 10, 2017

Coverage Status

Coverage decreased (-0.5%) to 64.228% when pulling 22f578d on feature/build-targets into 663359c on master.

class CustomError extends VError {
constructor() {
super(...arguments);
this.name = this.constructor.name;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work without babel transpiling? anything in the JS spec that says a constructor name equals the name of the subclass?

@@ -0,0 +1,11 @@
export default class Base {
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps we call it BaseModel to be more descriptive?

let exception = new NotImplementedError();
expect(NotImplementedError.matches(exception)).to.be.true;
})
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: Add tests for error chaining

@suda suda changed the title [WIP] Build targets support [Proposal] Unified Client interface Feb 15, 2017
@suda suda force-pushed the feature/build-targets branch from 22f578d to 65de0db Compare February 15, 2017 17:32
@coveralls
Copy link

coveralls commented Feb 15, 2017

Coverage Status

Coverage decreased (-0.5%) to 64.894% when pulling 65de0db on feature/build-targets into 6bd9caa on master.

@suda suda requested a review from busticated May 20, 2020 12:47
@suda
Copy link
Contributor Author

suda commented May 20, 2020

@busticated this is the stateful Client proposal I mentioned to you yesterday

@busticated busticated marked this pull request as draft November 17, 2020 00:28
@busticated
Copy link
Contributor

i moved this back into "draft" state to calm the reminders. i don't see us merging this anytime soon if only b/c it's so out of date but it's a good reference so i'm fine w/ leaving it open as a draft for now.

@busticated busticated removed their request for review November 17, 2020 00:30
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.

6 participants