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

[unidown] SWAPClient implementation with tests. Review at later date. #1680

Closed
wants to merge 7 commits into from
Closed

Conversation

sudk1896
Copy link
Contributor

My SWAPClient implementation got lost when I updated my swap branch to incorporate the latest changes from unidown. Its the same thing as my previous PR.
Review the changes at a later date.
@Cadair @dpshelio

@sudk1896 sudk1896 changed the title SWAPClient implementation with tests. Review at later date. [unidown] SWAPClient implementation with tests. Review at later date. Feb 26, 2016
@Cadair
Copy link
Member

Cadair commented Feb 26, 2016

@sudk1896 Can you add some tests of using the SWAPClient through the Fido interface?

@Cadair Cadair added the unidown label Feb 26, 2016
@sudk1896
Copy link
Contributor Author

@Cadair: Sure. Would do it in the next couple of days.

str19 str19 str6 str4
------------------- ------------------- ------ ----------
2015-12-28 00:00:00 2015-12-29 00:00:00 Proba2 swap
2015-12-29 00:00:00 2015-12-30 00:00:00 Proba2 swap]
Copy link
Member

Choose a reason for hiding this comment

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

This output is showing the wrong dates.

'%Y/%m/%d/'
'{instrument}_lv1_%Y%m%d_%H%M%S.fits')
crawler = Scraper(url_pattern, instrument= 'swap')
if not timerange:
Copy link
Member

Choose a reason for hiding this comment

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

What is the reasoning behind this? If timerange is not specified correctly this should error??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The earliest data available on the http://proba2.oma.be/swap/data/bsd/ is from SWAP_STARTDATE, so if the user specifies a date earlier than this, it would rightfully err.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, that's not what I mean, you are catching a case here where timerange is None or False, which is not valid input so should just error not be caught.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, It should throw an error. What error should I output ValueError, AttributeError etc ? I am thinking a simple exception saying input a valid timerange maybe.

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure it needs to be explicitly caught, Python will throw a ValueError (?) when you do the comparison to SWAP_STARTDATE.

Copy link
Member

Choose a reason for hiding this comment

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

it's generally good form to not catch things unless you need to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I have been testing it. If you don't pass time at all,
raise TypeError('{0!r}'.format(types))
If you pass an empty string in Time,
TypeError: __init__() takes at least 2 arguments (1 given)
If you pass timeranges in wrong order(start date greater than end date) it doesn't return an error or any result at all. So, I doubt why we should do the error checking there ?

Copy link
Member

Choose a reason for hiding this comment

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

yeah for Fido the Time attrr should do the error checking, that is on my todo list (see #1685 and #1668)

@sudk1896
Copy link
Contributor Author

Redid this as part of my Summer of Code. Closing this.

@sudk1896 sudk1896 closed this Jun 15, 2016
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

2 participants