-
Notifications
You must be signed in to change notification settings - Fork 32
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
Remove some broken code from collection.get
#21
Conversation
This section of code seems broken for a couple of reasons: 1. `Objects.keys` doesn't appear to take a second parameter. 2. The format of the resulting `requestObject.qs` property looks like it might be a little off: it looks like the intention here was maybe something more like `requestObject.qs = query`; If these assertions are correct, then I think `requestObject.qs` will always be `{ q: '' }` at the end of this code block. This doesn't seem very useful, hence this proposed change to remove it.
For reference, I was looking at this section of code while learning about how it works. My original goal was/is to try to implement and propose/contribute a solution to be able to getBy label selector (issue #2). I notice that there isn't a status check on PRs to run the tests. I also don't know how to run them locally (neither using the info from the README.md nor the test script from the package.json). Any help here would be greatly appreciated! |
@@ -157,13 +157,6 @@ var factory = module.exports = function (request) { | |||
|
|||
if (query && typeof query === 'object') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this PR is correct and accepted, the only thing left in this condition is to overwrite requestObject.endpoint
. If I'm correct, before it's overwritten here, it would be something like pods/[object Object]
, and this would overwrite it to pods
. I think this is a case that should be handled by the getPath
function itself, rather than needing to happen after it in certain circumstances, since the /[object Object]
part is likely never desirable. What do you think?
I think making the following change on line 11 would achieve this:
- return each !== null && typeof each !== 'undefined';
+ return typeof each !== 'object' && typeof each !== 'undefined';
In the above replacement, the initial null check would no longer be needed, since typeof null === 'object'
.
If you'd like, I can make that change in this PR (or in a separate one?)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also just move that initial definition of requestObject.endpoint down into an } else {
clause around line 167, so it isn't executed for the case where query
is an object.
@huangqg There has not been much activity on this repo for about 1.5 months. It would be great to see some progress on these pull requests. Would you be willing to grant someone else write access? If not perhaps we should start a new repo. |
qs.push(key + ':' + value); | ||
}); | ||
|
||
requestObject.qs = { q: qs.join(';') }; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my testing you'd want to have requestObject.qs = query;
here so that you can pass parameters like labelSelector
, etc. through to the underlying endpoint.
This section of code seems broken for a couple of reasons:
Objects.keys
doesn't appear to take a second parameter.requestObject.qs
property looks like itmight be a little off: it looks like the intention here was maybe
something more like
requestObject.qs = query
;If these assertions are correct, then I think
requestObject.qs
willalways be
{ q: '' }
at the end of this code block. This doesn't seemvery useful, hence this proposed change to remove it.