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

Fixes behaviour of get_stations() returning query objects #99

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

heavelock
Copy link

get_station() was returning query objects instead of list of station. The fix is simple, but why there were a workaround in the test? :(

@ThomasLecocq ThomasLecocq changed the title Fixes bug with get_stations() returning query objects Fixes behaviour of get_stations() returning query objects Oct 12, 2017
@ThomasLecocq
Copy link
Member

It's not really a bug, indeed, this is better like that. I will just check that this refactor doens't affect any other parts of the code (normally not).

@ThomasLecocq ThomasLecocq self-assigned this Oct 12, 2017
@ThomasLecocq ThomasLecocq added this to the 1.5.1 milestone Oct 12, 2017
@heavelock
Copy link
Author

heavelock commented Oct 12, 2017

Oh, okay. Due to that dualism in types being returned from that method, the calculate_stretching was failing. With that fix, it works.

Edit:
It looks like it breaks plot distance.
Well, I will check all the methods using it.

@heavelock
Copy link
Author

Okay, since most of the cases, this method returns a querry, maybe it should be better to make it returning the querry in all cases? And just converting the output according to needs.

I'm sorry I'm whining at this moment a bit, but returning different things from one method will definitely produce bugs in the future.

Now, instead of returning only lists or mix of lists and querries,
method get_stations() returns querries.
All methods requiring list was changed to obtain it.
@codecov
Copy link

codecov bot commented Oct 12, 2017

Codecov Report

Merging #99 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #99   +/-   ##
=======================================
  Coverage   74.28%   74.28%           
=======================================
  Files          17       17           
  Lines        2477     2477           
=======================================
  Hits         1840     1840           
  Misses        637      637
Impacted Files Coverage Δ
msnoise/test/tests.py 90.67% <100%> (ø) ⬆️
msnoise/api.py 63.59% <100%> (ø) ⬆️
msnoise/s02new_jobs.py 76.47% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f0f0ee...c6b6c7e. Read the comment docs.

@ThomasLecocq
Copy link
Member

Okay, since most of the cases, this method returns a querry, maybe it should be better to make it returning the querry in all cases? And just converting the output according to needs.

This is wrong, it only returns a query when the net arg is None, which, I guess, was a mistake from me. So no, I don't agree with the function returning a query, it's not intuitive for the user. The default behaviour of the API is to return user-friendly data, so in this case, it's a list of Station objects.

@heavelock
Copy link
Author

Okay. I will continue to dig into that bug and make this function return list.

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.

2 participants