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

renaming obspy.io.datamark to obspy.io.win #1717

Merged
merged 5 commits into from Mar 29, 2017

Conversation

Projects
None yet
4 participants
@ThomasLecocq
Contributor

ThomasLecocq commented Mar 13, 2017

What does this PR do?

This PR corrects my original mistake of calling the WIN format DATAMARK.
I refactored the code and the entry points but I didn't create deprecations.

It's linked to

  • #1692

  • #1494

  • All tests still pass.

  • Significant changes have been added to CHANGELOG.txt .

ThomasLecocq added some commits Mar 13, 2017

@ThomasLecocq

This comment has been minimized.

Show comment
Hide comment
@ThomasLecocq

ThomasLecocq Mar 13, 2017

Contributor

I guess codecov failure is due to moving/renaming.

Contributor

ThomasLecocq commented Mar 13, 2017

I guess codecov failure is due to moving/renaming.

@barsch

barsch approved these changes Mar 16, 2017

looks good for me

@megies

Although it's in general bad to rename modules without deprecation reroute.. I think we can make an exception as this is low level and should not be used directly and there is no gain for people to use low-level as opposed to read(..., format='DATAMARK')

@@ -142,7 +142,7 @@
'SLIST = obspy.io.ascii.core',
'PICKLE = obspy.core.stream',
'CSS = obspy.io.css.core',
'DATAMARK = obspy.io.datamark.core',
'WIN = obspy.io.win.core',

This comment has been minimized.

@megies

megies Mar 20, 2017

Member

Hmm.. In general I agree that we can make an exception here and allow the module move/renaming without deprecation reroute, as people have no reason to use the low level routines.

But.. we could still break peoples' codes if they circumvent the filetype detection, i.e. if they do read(..., format='DATAMARK').

Maybe we should leave the DATAMARK plugin in there (pointing to new io.win)?

Also, maybe we should wait with merging until it's clear how this one and #1692 will play together?

@megies

megies Mar 20, 2017

Member

Hmm.. In general I agree that we can make an exception here and allow the module move/renaming without deprecation reroute, as people have no reason to use the low level routines.

But.. we could still break peoples' codes if they circumvent the filetype detection, i.e. if they do read(..., format='DATAMARK').

Maybe we should leave the DATAMARK plugin in there (pointing to new io.win)?

Also, maybe we should wait with merging until it's clear how this one and #1692 will play together?

This comment has been minimized.

@ThomasLecocq

ThomasLecocq Mar 20, 2017

Contributor

would it work to state:

'DATAMARK = obspy.io.win.core',
'WIN = obspy.io.win.core',

?

@ThomasLecocq

ThomasLecocq Mar 20, 2017

Contributor

would it work to state:

'DATAMARK = obspy.io.win.core',
'WIN = obspy.io.win.core',

?

This comment has been minimized.

@megies

megies Mar 20, 2017

Member

And also the other plugin lines.. yeah.. not sure about any negative side effects, but probably it should be OK like this..

@megies

megies Mar 20, 2017

Member

And also the other plugin lines.. yeah.. not sure about any negative side effects, but probably it should be OK like this..

This comment has been minimized.

@ThomasLecocq

ThomasLecocq Mar 20, 2017

Contributor

or just a dummy obspy.io.datamark.core that answers it not longer exists and returns /dies. not a clean deprecation... but yah...

@ThomasLecocq

ThomasLecocq Mar 20, 2017

Contributor

or just a dummy obspy.io.datamark.core that answers it not longer exists and returns /dies. not a clean deprecation... but yah...

This comment has been minimized.

@megies

megies Mar 20, 2017

Member

that answers it not longer exists and returns /dies

If you do a deprecation, than it shouldn't raise but rather show a warning and use the new location at obspy.io.win

@megies

megies Mar 20, 2017

Member

that answers it not longer exists and returns /dies

If you do a deprecation, than it shouldn't raise but rather show a warning and use the new location at obspy.io.win

This comment has been minimized.

@ThomasLecocq

ThomasLecocq Mar 20, 2017

Contributor

... and stay until 1.2 ? pfff not sure...

@ThomasLecocq

ThomasLecocq Mar 20, 2017

Contributor

... and stay until 1.2 ? pfff not sure...

This comment has been minimized.

@megies

megies Mar 21, 2017

Member

If you do a deprecation, it will be in there for 1.1.* and removed again for 1.2.0

@megies

megies Mar 21, 2017

Member

If you do a deprecation, it will be in there for 1.1.* and removed again for 1.2.0

This comment has been minimized.

@megies

megies Mar 28, 2017

Member

@ThomasLecocq do you want to add a deprecation or leave as is? We plan to freeze 1.1.0 on Sunday so if this PR should be in there it should be ready by then.

@megies

megies Mar 28, 2017

Member

@ThomasLecocq do you want to add a deprecation or leave as is? We plan to freeze 1.1.0 on Sunday so if this PR should be in there it should be ready by then.

This comment has been minimized.

@ThomasLecocq

ThomasLecocq Mar 28, 2017

Contributor

I'd suggest to keep this as-is... I don't know many users of this routine anyway, and I've modified the docstring so that a simple googling should show DATAMARK in the io.win explanation

@ThomasLecocq

ThomasLecocq Mar 28, 2017

Contributor

I'd suggest to keep this as-is... I don't know many users of this routine anyway, and I've modified the docstring so that a simple googling should show DATAMARK in the io.win explanation

Show outdated Hide outdated obspy/io/win/README.txt
@@ -1,24 +1,28 @@
package obspy.io.datamark
package obspy.io.win
=========================

This comment has been minimized.

@megies

megies Mar 28, 2017

Member

header underline needs adjustment

@megies

megies Mar 28, 2017

Member

header underline needs adjustment

Update README.txt
markdown
+DOCS
@megies

megies approved these changes Mar 29, 2017

Alright, although this might break codes (eg >>> read(..., format='DATAMARK'), at least it for sure won't break a lot of peoples' codes.. ;-) so I guess we can merge this..

And the moral of the story.. alwys try to get module names right with the first try.. ;-)

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Mar 29, 2017

Member

And the moral of the story.. alwys try to get module names right with the first try.. ;-)

Yea ;-)

Looks good to me. We just have to remember to document this in some sense breaking chance in the release message.

Member

krischer commented Mar 29, 2017

And the moral of the story.. alwys try to get module names right with the first try.. ;-)

Yea ;-)

Looks good to me. We just have to remember to document this in some sense breaking chance in the release message.

@krischer krischer merged commit 111ee34 into obspy:master Mar 29, 2017

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
docker-deb-buildbot Deb packaging and testing succeeded
Details
docker-testbot Docker tests succeeded
Details

@krischer krischer deleted the ThomasLecocq:rename_datamark_win branch Mar 29, 2017

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