Fix redundant parameters

[zanetworker]

Redundant parameters issue fix #33

[broadbent]

@zanetworker, do you think it would be better to pass the additional parameters (and keep the 'get' in the RPC handler), and instead delete the field from the passed 'options'? I think this would avoid the redundancy and keep the separation in functionality. Comments?

[zanetworker]

@broadbent I believe removing the "fields" is a better option because in the specification, only three parameters are passed to the create method, namely "type, credentials and options". Introducing a new fields parameter for delegate calls might be a bit confusing. Besides, I see what you are saying but I think both approaches address the same objective. Otherwise, can you elaborate more on the advantages? I don't understand "the separation of functionality" here, what is the extra functionality that the 'fields' parameter provides? Besides, the options is meant to contain 'fields' as described in the specification. """

options:

'fields', a dictionary field/value pairs for object to be created

"""

[broadbent]

@zanetworker, I agree that in the specification, only 'options' are given during an RPC call. However, beyond the RPC handler, we are firmly within our own implementation, so there is no need to keep the parameters consistent.

The 'separation of functionality' comment is more about how we structure our code. The handler does some simple retrieval and formatting of the RPC parameters. This functionality is grouped in the handler, and passed as individual function parameters ('fields' for example) to keep the delegate functions simple (the delegate class does none of this itself).

I would suggest we keep it this way, and do this 'pre-parsing' in the handler. Ideally though, we would want to remove the fields we have retrieved from the 'options' parameter to avoid passing duplicated data. It would still be useful to pass the 'options' in the case that the delegate needs to parse any further options that may be present (this isn't the case currently, but may change with API revisions).

[zanetworker]

@broadbent I do see your point. In fact, this is the reason we posted this issue in the first place. Although we want to keep the delegate functions simple, reading the delegate functions as they are now raises the question "why do we need fields again", even though we might have removed the "fields" parameter from "options". As an outsider who understands the specification but reads the code, it will be confusing to understand why the "fields" parameter is being passed. Anyhow, for me it is not a big deal to do the change, but do you get what I mean?

[broadbent]

@zanetworker I think the solution is to update the documentation and make it clear what the parameters represent. In particular, we should document where the value is derived from.

As a side note, there seems to be a couple of additional modules added to the pip requirements file. However, they don't seem to be used in the code. Do we need them?

[zanetworker]

@broadbent yea that could be another solution.

Regarding the extra modules in the requirements file, I just did another commit, and I removed all the un-necessary modules. I just did pip freeze and copied the output into the requirements and these extra package were there. Anyhow, they are not needed and they were removed.

[motine]

I am unsure what the verdict is now. I would vote for all pre-processing (incl. extracting fields) in the RPC and not in the Delegate.

[zanetworker]

@motine that is what was done in the first place before adding the issue, the 'fields' parameter was extracted and passed as an extra parameter in addition to options in the handler. However, the title of the issue is to remove redunduncy as we don't want to have fields passed because we can get it from options. In this pull request I remove the extra fields parameter (the redunduncy).

[motine]

@zanetworker Sorry, I did not make myself clear from the start (I am really sorry). We should have the fields parameter in the Delegate (so all pre-processing is done in the RPC Handler and the implicit arguments are made explicit). The fields key in the options parameter needs to be removed when calling the Delegate method within the Handler. Again, sorry for me not being clear from the start and hence wasting your time.

[zanetworker]

@motine No problem :)

[broadbent]

@zanetworker @motine I think we have reached a consensus in terms of this pull request. @zanetworker is going to refactor the request so that all pre-processing is done in the RPC handler, and duplicate fields are removed from 'options'.

[motine]

Sorry, I did not see that you created #33 and the pull request looked like it would remove the fields parameter.