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

Attr dict type check #1997

Merged
merged 16 commits into from Nov 23, 2017

Conversation

Projects
None yet
3 participants
@d-chambers
Member

d-chambers commented Nov 12, 2017

What does this PR do?

This PR enables obspy.core.util.AttribDict and subclasses to perform basic type checks when assigning values to names defined in the _types dict, which stores the name of the attribute as a key and its type as the value (tuples of types also work). If the correct type was not passed a warning will be issued and the value will be cast into the stated type (or first type if tuple is used).

It is then applied on obspy.core.trace.Stats to ensure a string or None is passed to the network, station, location, and channel codes.

For example:

from obspy.core.trace import Stats
stats = Stats()

stats.network = 'UU'  # this works fine just as before
stats.station = 1  # this issues a warning that station must be a str and casts value to '1'
stats.location = None  # also works fine

Why was it initiated? Any relevant Issues?

See: #1995

PR Checklist

  • This PR is not directly related to an existing issue (which has no PR yet).
  • All tests still pass.
  • Any new features or fixed regressions are be covered via new tests.
  • Any new or changed features have are fully documented.
  • Significant changes have been added to CHANGELOG.txt .
  • First time contributors have added your name to CONTRIBUTORS.txt .

Additional Suggestions:

Additionally, I suggest the following changes to the AttribDict class:

  1. Replace all lists that exist solely for membership checks with sets or tuples. This adds the advantage of more efficient look-ups or immutability (either one seems to be an advantage over a list).

This would effect readonly, and do_not_warn_on.

  1. Rename all class level attributes that can affect item/attr assignment to begin with an underscore. This would send a clear message that users should not alter them. This would include defaults, readonly, warn_on_non_default_key and do_not_warn_on. An unlikely, but possible edge-case as it stands now would allow the user to do something like the following, while only accessing "public" attributes:
from obspy.core.util import AttribDict
ad = AttribDict()
ad.readonly = True  # this breaks the core functionality of the AttribDict
ad.some_value = 13  # raises TypeError because bool is not iterable

if, however, the user had to do this:

ad._readonly = True

it would be clear that non-public attrs were being messed with. Should I open another issue or should we discuss here?

edit: I just remembered in python sets are not immutable, updated comment accordingly.

@krischer

Looks mostly good to me. Thanks!

The only things which I'm currently worried about a bit is how it deals with the different string types in Python 2 and 3. Can you please add a test that uses the future package's native_string and native_bytes types + also the unicode type and test what happens? This is such a core piece of ObsPy and we have to get it right.

Show outdated Hide outdated obspy/core/trace.py
Show outdated Hide outdated obspy/core/util/attribdict.py
Show outdated Hide outdated obspy/core/tests/test_stats.py
"""
Test that types are enforced with _types attribute
"""
class AttrOcity(AttribDict):

This comment has been minimized.

@krischer

krischer Nov 12, 2017

Member

:-) Nice name!

@krischer

krischer Nov 12, 2017

Member

:-) Nice name!

Show outdated Hide outdated obspy/core/tests/test_util_attribdict.py
@d-chambers

This comment has been minimized.

Show comment
Hide comment
@d-chambers

d-chambers Nov 13, 2017

Member

I wrote a test that ensures the behavior I was after in the first place: ensuring the nslc values in Stats objects do not prevent a stream from being saved as mseed. It goes something like this:

    def test_casted_stats_nscl_still_writes(self):
        """
        Ensure a Stream object that has had its nslc types cast to str can
        still be written as mseed. 
        """
        st = Stream(traces=read()[0])

        # set new stats
        with warnings.catch_warnings(record=True) as w:
            warnings.simplefilter('default')
            st[0].stats.network = 1
            st[0].stats.station = 1.1
            st[0].stats.location = b'23'
            st[0].stats.channel = None

        # try writing stream to io
        bio = io.BytesIO()
        st.write(bio, 'mseed')

However, st.write fails because None (stats.channel) has not attribute encode. Since the default value for the nslc codes is a blank string I don't think None should be an option (not sure why I added it). I will change the none test and remove None from the supported types dict. The way I was getting at NoneType was a bit kludgey anyhow.

Member

d-chambers commented Nov 13, 2017

I wrote a test that ensures the behavior I was after in the first place: ensuring the nslc values in Stats objects do not prevent a stream from being saved as mseed. It goes something like this:

    def test_casted_stats_nscl_still_writes(self):
        """
        Ensure a Stream object that has had its nslc types cast to str can
        still be written as mseed. 
        """
        st = Stream(traces=read()[0])

        # set new stats
        with warnings.catch_warnings(record=True) as w:
            warnings.simplefilter('default')
            st[0].stats.network = 1
            st[0].stats.station = 1.1
            st[0].stats.location = b'23'
            st[0].stats.channel = None

        # try writing stream to io
        bio = io.BytesIO()
        st.write(bio, 'mseed')

However, st.write fails because None (stats.channel) has not attribute encode. Since the default value for the nslc codes is a blank string I don't think None should be an option (not sure why I added it). I will change the none test and remove None from the supported types dict. The way I was getting at NoneType was a bit kludgey anyhow.

@d-chambers

This comment has been minimized.

Show comment
Hide comment
@d-chambers

d-chambers Nov 13, 2017

Member

Any thoughts on the additional suggestions I made in the first PR post? Worth the change or too risky?

Member

d-chambers commented Nov 13, 2017

Any thoughts on the additional suggestions I made in the first PR post? Worth the change or too risky?

@d-chambers

This comment has been minimized.

Show comment
Hide comment
@d-chambers

d-chambers Nov 13, 2017

Member

AppVeyor is failing on python 2.7 now, will look into later this week.

Member

d-chambers commented Nov 13, 2017

AppVeyor is failing on python 2.7 now, will look into later this week.

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Nov 13, 2017

Member

However, st.write fails because None (stats.channel) has not attribute encode. Since the default value for the nslc codes is a blank string I don't think None should be an option (not sure why I added it). I will change the none test and remove None from the supported types dict. The way I was getting at NoneType was a bit kludgey anyhow.

I see and also did not consider this in my first review round. I second removing None from the supported types dict.

Otherwise this now looks fine to me once CI passes.

Member

krischer commented Nov 13, 2017

However, st.write fails because None (stats.channel) has not attribute encode. Since the default value for the nslc codes is a blank string I don't think None should be an option (not sure why I added it). I will change the none test and remove None from the supported types dict. The way I was getting at NoneType was a bit kludgey anyhow.

I see and also did not consider this in my first review round. I second removing None from the supported types dict.

Otherwise this now looks fine to me once CI passes.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Nov 16, 2017

Member

I think since we already touch this.. we might want to protect all of those special attributes from accidental overwrites? Maybe use two leading underscores and add getter properties for them?

Member

megies commented Nov 16, 2017

I think since we already touch this.. we might want to protect all of those special attributes from accidental overwrites? Maybe use two leading underscores and add getter properties for them?

@d-chambers

This comment has been minimized.

Show comment
Hide comment
@d-chambers

d-chambers Nov 16, 2017

Member

protect all of those special attributes from accidental overwrites? Maybe use two leading underscores and add getter properties for them?

@megies, personally I dislike the double underscore name mangling thing python does. A single underscore should be enough to tell users "don't mess with this unless you know what you are doing".

It would be easy to make a set of forbidden attrs, say (_defaults, _readonly, _warn_on_non_default_key, _do_not_warn_on) then ensure an exception is raised when a user tries to set one of these attributes.

Member

d-chambers commented Nov 16, 2017

protect all of those special attributes from accidental overwrites? Maybe use two leading underscores and add getter properties for them?

@megies, personally I dislike the double underscore name mangling thing python does. A single underscore should be enough to tell users "don't mess with this unless you know what you are doing".

It would be easy to make a set of forbidden attrs, say (_defaults, _readonly, _warn_on_non_default_key, _do_not_warn_on) then ensure an exception is raised when a user tries to set one of these attributes.

@d-chambers

This comment has been minimized.

Show comment
Hide comment
@d-chambers

d-chambers Nov 18, 2017

Member

So I tried renaming some of these attributes to start with a leading underscore but so many things broke when I ran the tests (>50). I am starting to think it may not be feasible at this point to change them. Even if I get all the tests passing who knows how many user code this would break...

Member

d-chambers commented Nov 18, 2017

So I tried renaming some of these attributes to start with a leading underscore but so many things broke when I ran the tests (>50). I am starting to think it may not be feasible at this point to change them. Even if I get all the tests passing who knows how many user code this would break...

@krischer

Looks mostly good to me know. Just some minor comments remain.

Show outdated Hide outdated obspy/core/tests/test_stats.py
Show outdated Hide outdated obspy/core/tests/test_stats.py
Show outdated Hide outdated obspy/core/trace.py
Show outdated Hide outdated obspy/core/util/attribdict.py
@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Nov 20, 2017

Member

So I tried renaming some of these attributes to start with a leading underscore but so many things broke when I ran the tests (>50). I am starting to think it may not be feasible at this point to change them. Even if I get all the tests passing who knows how many user code this would break...

I agree - if its too hard or invasive best don't change it in this PR here.

Member

krischer commented Nov 20, 2017

So I tried renaming some of these attributes to start with a leading underscore but so many things broke when I ran the tests (>50). I am starting to think it may not be feasible at this point to change them. Even if I get all the tests passing who knows how many user code this would break...

I agree - if its too hard or invasive best don't change it in this PR here.

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Nov 23, 2017

Member

Thanks a lot for responding to all the nagging of mine. I'm happy now :) If no other comments show up I'll merge this by the end of today.

Member

krischer commented Nov 23, 2017

Thanks a lot for responding to all the nagging of mine. I'm happy now :) If no other comments show up I'll merge this by the end of today.

@krischer krischer merged commit 6dfaf4e into master Nov 23, 2017

5 of 6 checks passed

docker-testbot docker testbot results not available yet
ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 90%)
Details
codecov/project 88.06% (+1.32%) compared to 0ed7a93
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@krischer krischer deleted the attr_dict_type_check branch Nov 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment