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

Fix instance vs class variables in FileReader #122

Merged
merged 1 commit into from
Dec 5, 2017

Conversation

llimeht
Copy link
Contributor

@llimeht llimeht commented Nov 2, 2017

Class variables get inherited and shared between different instantiations
of the class and this means that the current usage of TestFileReader in
utest_generic_file_reader_class.py has side-effects that would cause
subsequent tests to fail.

(See an upcoming PR for why being able to run lots of tests at once is useful)

Copy link
Contributor

@pkienzle pkienzle left a comment

Choose a reason for hiding this comment

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

Suggest leaving a place holder for the attributes in the class, preferably with PEP 484 type hinting since IDEs will eventually be able to use it. Also, attach attribute descriptions in a way that sphinx can document them.
Not so valuable on the test infrastructure, but useful habits for long term maintenance of the code base.

E.g.,

class FileReader(object):
    #: List of Data1D and Data2D objects to be sent back to the data loader
    output = None  # type: List[Union[Data1D, Data2D]]
    ...

Regarding this particular commit, only the output attribute really needs to be initialized in init since it refers to a mutable structure. You could instead use the following at the class level:

    output = ()  # type: Sequence[Union[Data1D, Data2D]]

which has the advantage of acting like a sequence if it is not initialized, but being non-mutable, you won't accidentally share state between instances.

@llimeht
Copy link
Contributor Author

llimeht commented Nov 3, 2017

It seems unlikely that current_dataset, current_datainfo or f_open should be shared between readers either. They sound like (and are used like) instance variables not class variables. Shadowing a class variable with an instance variable by redefining output definitely doesn't feel like the right thing to do either. If the goal is make life easier for users of the API and providing hints to IDEs, then mixing class and instance variables isn't the right thing to do. A decent editor can find self.foo in the constructor and will also learn that it is supposed to be an instance variable at the same time (to be accessed via self.foo and not MyClass.foo).

Type annotations are a nice next addition to the code although I don't think I can infer the types myself from the code that is there to write them. (All I did was hit tab, after all!) I'll look at that later.

@pkienzle
Copy link
Contributor

pkienzle commented Nov 3, 2017

I'm not suggesting that class attributes be used for instance information, only for non-mutable default values and as a place-holder for documentation and type information. The code doesn't care if foo is the class default or an instance specific value when you write self.foo.

You can get the same effect by moving the declaration to be within init (though the default value will be missing from the docs), so the decision is largely stylistic. Since the style guide [http://trac.sasview.org/wiki/DevNotes/DevGuide/CodingRules] doesn't say anything, I guess that leaves it up to the author. I switched to class level declarations when learning mypy and find it convenient since it puts all public attributes at the top of the class definition rather than spreading them throughout the initialization code, but YMMV.

Regarding type annotation, this is aspirational. At one point sasmodels was mostly annotated, but I never went so far as enforcing it with a mypy check as part of the build, so I'm sure it has degraded a bit. We have not even begun to do so for sasview.

Class variables get inherited and shared between different instantiations
of the class and this means that the current usage of TestFileReader in
utest_generic_file_reader_class.py has side-effects that would cause
subsequent tests to fail.
@krzywon krzywon merged commit 79c9ce5 into SasView:master Dec 5, 2017
@llimeht llimeht deleted the tmp/filereader-class branch January 19, 2018 23:08
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