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

Update time.py Removed extract_time #2029

Merged
merged 6 commits into from
Mar 14, 2017
Merged

Conversation

prateekiiest
Copy link
Contributor

@prateekiiest prateekiiest commented Mar 9, 2017

@dpshelio , added a deprecated decorator.
The function should now issue a warning if invoked.

issue #2024

@pep8speaks
Copy link

pep8speaks commented Mar 9, 2017

Hello @prateekiiest! Thanks for updating the PR.

Line 206:1: E303 too many blank lines (3)

Line 124:1: E303 too many blank lines (4)

Comment last updated on March 09, 2017 at 14:44 Hours UTC

@prateekiiest
Copy link
Contributor Author

Or is it required to totally remove the extract_time function??

ping @Cadair @dpshelio

@prateekiiest prateekiiest changed the title Update time.py Update time.py Removed extract_time Mar 9, 2017
Copy link
Contributor Author

@prateekiiest prateekiiest left a comment

Choose a reason for hiding this comment

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

I removed the function instead of using deprecated

@m-charlton
Copy link
Contributor

Cheers

@Cadair
Copy link
Member

Cadair commented Mar 9, 2017

The test is going to need to be removed and it's going to need a changelog.

@prateekiiest
Copy link
Contributor Author

So all the tests including
test_extract_time_illegal_args():
test_extract_time_best_effort()
must be removed , since they use extract_time().

Am I right?

@prateekiiest prateekiiest mentioned this pull request Mar 9, 2017
@Cadair
Copy link
Member

Cadair commented Mar 9, 2017

yes, anything that calls extract time will need removing.

@prateekiiest
Copy link
Contributor Author

Done @Cadair

@prateekiiest
Copy link
Contributor Author

Done removing the tests that call extract_time.

@Cadair
Copy link
Member

Cadair commented Mar 9, 2017

Needs a change log due to the API change :)

@prateekiiest
Copy link
Contributor Author

OK, on the way.

@prateekiiest
Copy link
Contributor Author

@Cadair
Is it ok, now?

@Cadair
Copy link
Member

Cadair commented Mar 9, 2017

The tests have failed. I am going to guess you haven't removed extract_time from the __all__ list somewhere. Check the build logs and run the tests locally before you push.

Removed extract_time from _all_
Copy link
Contributor Author

@prateekiiest prateekiiest left a comment

Choose a reason for hiding this comment

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

Removed extract_time from all

Copy link
Contributor Author

@prateekiiest prateekiiest left a comment

Choose a reason for hiding this comment

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

Removed extract_time from import

@Cadair Cadair merged commit 4b20c49 into sunpy:master Mar 14, 2017
@prateekiiest prateekiiest deleted the patch-2 branch March 15, 2017 06:25
@Cadair Cadair mentioned this pull request Mar 21, 2017
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

5 participants