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

[WIP] #5 #7

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[WIP] #5 #7

wants to merge 5 commits into from

Conversation

viniazvd
Copy link

No description provided.

@lubien lubien changed the title "[WIP] #5" [WIP] #5 Dec 14, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 97.674% when pulling 2027bc0 on viniazvd:master into 77ffd1e on jay-ess:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 97.674% when pulling 2027bc0 on viniazvd:master into 77ffd1e on jay-ess:master.

Copy link
Contributor

@lubien lubien left a comment

Choose a reason for hiding this comment

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

This commit should disappear.

@@ -4,3 +4,4 @@
"add-module-exports"
]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

No need to this. Please refrain.

@@ -8,7 +8,7 @@
"test": "test"
},
"scripts": {
"test": "xo && ava test/**/*.test.js --require babel-register",
"test": "xo && ava test/**/*.test.js --require babel-register --verbose",
Copy link
Contributor

Choose a reason for hiding this comment

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

If needed to do this, do in a commit only for this, explaining why.

Copy link
Author

Choose a reason for hiding this comment

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

you are right, sry.

src/container.js Outdated
@@ -1,45 +1,44 @@
const debug = require('debug')('restinga:container');
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole file diff is wrong. Editing the container isn't intended. We need to remove it.

}
}
module.exports = function ({service, prefix, agent}) {
this.service = service;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't do this in a function.

Prefer to return a object.

Copy link
Author

Choose a reason for hiding this comment

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

understood!

Copy link
Contributor

@lubien lubien left a comment

Choose a reason for hiding this comment

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

Please write a proper name. 'Delete Container', if the case.

Use 'Container' as we're talking about a class.

@@ -1,5 +1,5 @@
import Request from './request';
import {Container} from './';
// import descriptor from './descriptor';
Copy link
Contributor

Choose a reason for hiding this comment

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

No comment here.

Copy link
Author

Choose a reason for hiding this comment

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

ok, i had already removed the comments on the last commit.

src/resource.js Outdated
@@ -25,7 +25,8 @@ export default class Resource {
this.collectionRoot = collectionRoot;
this.itemRoot = itemRoot;

this.descriptor = Container.get(this.service);
const obj = {service: 'json-placeholder', prefix: 'http://jsonplaceholder.typicode.com', agent: 'restinga-node/1.0.0 (https://github.com/jay-ess/restinga-node)'};
Copy link
Contributor

@lubien lubien Dec 14, 2017

Choose a reason for hiding this comment

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

Name it const descriptor to be more clear.

Don't make it a one-liner as it's too big.

Copy link
Author

Choose a reason for hiding this comment

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

done

src/resource.js Outdated
@@ -25,7 +25,8 @@ export default class Resource {
this.collectionRoot = collectionRoot;
this.itemRoot = itemRoot;

this.descriptor = Container.get(this.service);
const obj = {service: 'json-placeholder', prefix: 'http://jsonplaceholder.typicode.com', agent: 'restinga-node/1.0.0 (https://github.com/jay-ess/restinga-node)'};
this.descriptor = obj || null;
Copy link
Contributor

Choose a reason for hiding this comment

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

You're not receiving a descriptor from the user, you're mocking one!

Copy link
Author

Choose a reason for hiding this comment

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

i know, for while.

@@ -1,10 +1,10 @@
import test from 'ava';
import {Container} from '../src';
// import {Container} from '../src';
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment, again.

import {JsonPlaceholderService, PostResource, CommentResource} from './fixtures';

const jsonPlaceholderService = new JsonPlaceholderService();

Container.register(jsonPlaceholderService);
// Container.register(jsonPlaceholderService);
Copy link
Contributor

Choose a reason for hiding this comment

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

Again.

@lubien
Copy link
Contributor

lubien commented Dec 14, 2017

Don't forget I asked for the commits deletion.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 97.638% when pulling f33d5df on viniazvd:master into 77ffd1e on jay-ess:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 97.638% when pulling f33d5df on viniazvd:master into 77ffd1e on jay-ess:master.

@coveralls
Copy link

coveralls commented Dec 14, 2017

Coverage Status

Coverage decreased (-0.2%) to 97.638% when pulling 3e0d921 on viniazvd:master into 77ffd1e on jay-ess:master.

@coveralls
Copy link

coveralls commented Dec 14, 2017

Coverage Status

Coverage decreased (-0.2%) to 97.638% when pulling b7cd31b on viniazvd:master into 77ffd1e on jay-ess:master.

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.

None yet

3 participants