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

Master is now Py >=3.6 ONLY #2655

Merged
merged 20 commits into from Jun 18, 2018

Conversation

Projects
None yet
5 participants
@nabobalis
Copy link
Contributor

commented Jun 14, 2018

Parts of this PR:

  1. Moved to upstream/master version of astropy_helpers
  2. Moved Figure tests to Py3.6
  3. Removed old metaclasses for Map and Time Series now we can use Py3.6 only magic.
  4. General expansion/update of the gitignore
  5. NOT ANYMORE
  6. Probs other things I forget now.

@nabobalis nabobalis added this to the 1.0 milestone Jun 14, 2018

@sunpy-bot

This comment has been minimized.

Copy link

commented Jun 14, 2018

Thanks for the pull request @nabobalis! Everything looks great!

@sunpy sunpy deleted a comment from sunpy-bot bot Jun 14, 2018

@nabobalis nabobalis force-pushed the nabobalis:meta_class branch from 820b467 to 8ecfbe9 Jun 14, 2018

@Cadair

Cadair approved these changes Jun 14, 2018

@@ -175,6 +136,13 @@ class GenericMap(NDData):
rest will be discarded.
"""

_registry = dict()

def __init_subclass__(cls, **kwargs):

This comment has been minimized.

Copy link
@Cadair

Cadair Jun 14, 2018

Member

Given this is pretty arcane and also a very new Python feature a good docstring describing what this is doing in general and for us is probably needed.

This comment has been minimized.

Copy link
@nabobalis

nabobalis Jun 14, 2018

Author Contributor

Fixed. ON THE LINE BELOW.

@@ -111,6 +73,13 @@ class GenericTimeSeries:

# Class attribute used to specify the source class of the TimeSeries.
_source = None
_registry = dict()

def __init_subclass__(cls, **kwargs):

This comment has been minimized.

Copy link
@Cadair

Cadair Jun 14, 2018

Member

same as above

This comment has been minimized.

Copy link
@nabobalis

nabobalis Jun 14, 2018

Author Contributor

same as above

@@ -565,6 +565,5 @@ class NoTimeSeriesFound(ValueError):
pass


TimeSeries = TimeSeriesFactory(default_widget_type=GenericTimeSeries,
TimeSeries = TimeSeriesFactory(registry= GenericTimeSeries._registry, default_widget_type=GenericTimeSeries,

This comment has been minimized.

Copy link
@Cadair

Cadair Jun 14, 2018

Member

rouge space

@Cadair
Copy link
Member

left a comment

needs moar docs

@sunpy sunpy deleted a comment from pep8speaks Jun 14, 2018

@Cadair

This comment has been minimized.

Copy link
Member

commented Jun 14, 2018

@pep8speaks quiet

@Cadair Cadair requested a review from dstansby Jun 14, 2018

@Cadair

This comment has been minimized.

Copy link
Member

commented Jun 14, 2018

@ayshih underneath the other changes in here is some changes to the metaclass stuff that you might be interested in reviewing.

import io
import locale
import os
import re
import subprocess as sp
import sys

__minimum_python_version__ = (3, 5)

This comment has been minimized.

Copy link
@dstansby

dstansby Jun 14, 2018

Contributor

Should this be (3, 6)? (is this file just an update from an external source?)

This comment has been minimized.

Copy link
@Cadair

Cadair Jun 15, 2018

Member

yeah this is just an update from astropy_helpers, we will add our own version of this when I get around to it lol.

@@ -0,0 +1,4 @@
Removed old metaclass used for Map and TimeSeries as we have now moved to Python 3.6.
Moved figure tests to 3.6.

This comment has been minimized.

Copy link
@dstansby

dstansby Jun 14, 2018

Contributor

"3.6" > "Python 3.6"

@@ -0,0 +1,26 @@
name: sunpy-figure-tests-3.6

This comment has been minimized.

Copy link
@dstansby

dstansby Jun 14, 2018

Contributor

Is it possible to keep the requirements the same as before, but just bump the python version? Then we can start bumping e.g. Matplotlib requirements when I have image comparison working and we can see what the changes are.

This comment has been minimized.

Copy link
@nabobalis

nabobalis Jun 15, 2018

Author Contributor

I didn't think to do that. I just updated my current env, saved it to a file and then updated the versions manually.

The image comparison I used was the figures on your repo, everything bar the issue I opened looked the same dispite the hash change.

This comment has been minimized.

Copy link
@nabobalis

nabobalis Jun 18, 2018

Author Contributor

I did as you suggested. The hashes are the same but new python version. So we have just done that and updated the env that way with no other changes.

@nabobalis nabobalis force-pushed the nabobalis:meta_class branch from c9d1690 to d7c7804 Jun 15, 2018

@Cadair

This comment has been minimized.

Copy link
Member

commented Jun 18, 2018

We should move to a tagged version of astropy_helpers, I.e. a release.

@nabobalis nabobalis force-pushed the nabobalis:meta_class branch from f635c6e to b088eab Jun 18, 2018

* Should we use Helioviewer or VSO's data model? (e.g. map.meas, map.wavelength
or something else?)
* Should 'center' be renamed to 'offset' and crpix1 & 2 be used for 'center'?
"""

This comment has been minimized.

Copy link
@nabobalis

nabobalis Jun 18, 2018

Author Contributor

What should we do about these?

This comment has been minimized.

Copy link
@Cadair

Cadair Jun 18, 2018

Member

They are super old we can ditch them

from __future__ import absolute_import

from abc import ABCMeta, abstractmethod
from abc import ABC, abstractmethod

This comment has been minimized.

Copy link
@Cadair

Cadair Jun 18, 2018

Member

this should be below the os import

* Should we use Helioviewer or VSO's data model? (e.g. map.meas, map.wavelength
or something else?)
* Should 'center' be renamed to 'offset' and crpix1 & 2 be used for 'center'?
"""

This comment has been minimized.

Copy link
@Cadair

Cadair Jun 18, 2018

Member

They are super old we can ditch them

@@ -2,10 +2,10 @@
# Lovingly borrowed from Astropy
# Licensed under a 3-clause BSD style license - see licences/ASTROPY.rst

import warnings

This comment has been minimized.

Copy link
@Cadair

Cadair Jun 18, 2018

Member

these should be where they were before so they are in length then alphabetical order. #fussy

nabobalis added some commits Jun 18, 2018

@Cadair

Cadair approved these changes Jun 18, 2018

Copy link
Member

left a comment

Awesome!

@ayshih

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2018

@ayshih underneath the other changes in here is some changes to the metaclass stuff that you might be interested in reviewing.

@Cadair I like it!

@nabobalis

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2018

@ayshih I will take that as an approval of the PR!

@ayshih

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2018

You can if you wish. =) I only really looked at the metaclass stuff, but everything else seems fine at first look.

@nabobalis

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2018

Nope, Cadair says I need a second review so come on and step up @sunpy/sunpy-developers!

@vn-ki

vn-ki approved these changes Jun 18, 2018

Copy link
Member

left a comment

Looks good to me. 🥂

@Cadair Cadair merged commit 9df219a into sunpy:master Jun 18, 2018

9 checks passed

Giles Click details to preview the documentation build
Details
ci/circleci: egg-info-36 Your tests passed on CircleCI!
Details
ci/circleci: figure-tests-36 Your tests passed on CircleCI!
Details
ci/circleci: html-docs Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 71.84%)
Details
codecov/project 83.35% (+11.51%) compared to 60ddbe1
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sunpy-bot All checks passed

@nabobalis nabobalis deleted the nabobalis:meta_class branch Jun 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.