-
-
Notifications
You must be signed in to change notification settings - Fork 211
Description
Currently our python objects tend to have many attributes that we only want modified by the API itself, e.g. all of the reference to ids. However, by not prefixing those attributes or making them read-only, we don't signal clearly to the users that they should not be tampered with.
At the same time (partially because of this?), we introduce a lot of extra work to test behavior which should only occur if the user uses the API in bad faith, e.g. manually overwriting ids.
PEP8 proposes that non-public attributes be prefixed with _. There is also the option of creating read-only properties like such:
class OpenMLRun:
def __init__(self, run_id):
self._run_id = run_id
@property
def run_id(self):
return self._run_id
Disadvantages to prefixing without a property:
- all old code with read and write operations breaks
- need to access 'non-public' attributes for run_id (for the correct use of reading the value).
Disadvantages to prefixing with a property:
- all old code with write operations breaks
- verbose as our classes may have many of attributes which should be read-only for outside users
Disadvantages of leaving as is:
- more prone to users accidentally writing fields that they should not do
- maintaining unit tests to test behavior in case those mistakes are made
There is of course also the opportunity of leaving things as they are without proper unit tests.
This might give some weird local behavior when things are done that shouldn't be done, but server-side there shouldn't be any issues (after all, you should be able to throw anything at the REST API directly anyway, right?).
What are your thoughts?
I feel that we should move in a direction which avoids creating/maintaining unit tests for users deliberately playing with our internals.