Skip to content
This repository has been archived by the owner on May 13, 2020. It is now read-only.

Python 2/3 compatibility in Python code, streamline install doc, implement continuous integration, vastly simplify install #312

Closed
wants to merge 22 commits into from

Conversation

scivision
Copy link

@scivision scivision commented Jun 21, 2017

  • C code still uses obsolete calls to Python lib, that is blocker for Python 3.
  • removed some syntax errors in code

@scivision scivision changed the title Python 2/3 compatibility in Python code, streamline install doc Python 2/3 compatibility in Python code, streamline install doc, implement continuous integration, vastly simplify install Jun 21, 2017
@asreimer
Copy link
Contributor

This is awesome. I've spent time looking through all the edits and I don't see a single thing that will break our code.

I will perform some code tests in the coming days. I encourage the other developers to do the same!

@aburrell
Copy link
Contributor

It would be lovely to get this incorporated before our next release. Have any tests been developed?

@scivision
Copy link
Author

If tests haven't been developed in time, at least the non-risky stuff like setup.py being made nice etc. could be done.
What is timeline for next release and will @asreimer have time?

@aburrell
Copy link
Contributor

@asreimer is writing up and working outside of superDARN at the moment, so probably not. The next release is scheduled for AGU.

Dividing this up to separate the setup.py changes and python 2/3 changes is probably the best way to go.

@asreimer
Copy link
Contributor

asreimer commented Oct 13, 2017

It's nice if pull requests have separate functions, but this is so straight forward (by that I mean reading the changes, I don't see any cryptic modifications to the code) that it seems like it might just be extra work at this point and delay it further than already has been.

I did some testing back in June/July, but didn't post. I'll try to have a look at this on the weekend.

I'm sorry about my absence. I've been monitoring though. The issue isn't that I'm working outside SuperDARN. The issue is that I'm writing my thesis (which is a sponge on my free time).

It's also not an issue of developing tests to get this merged. People should test their own code on this branch. Until we get unit testing merged in, we have no standard testing. So I suggest people just test the branch, same as we do for other pull requests. Yes, usually a pull request comes with specific code to run, but this pull request touches everything, so we need to test everything.

@asreimer
Copy link
Contributor

asreimer commented Oct 14, 2017

Ok, I resolved all the conflicts. I don't have time right now to post the testing code, but I will later this weekend.

There's no reason for people not to test this code now that I fixed the conflicts :)

Here's how you can get a branch set up to test the code:

git checkout develop
git pull origin develop
git checkout -b scivision-develop
git remote add scivision-davitpy https://github.com/scivision/davitpy.git
git fetch scivision-davitpy
git pull scivision-davitpy develop

And then you should start testing everything you can. Running setup.py, running all the unit tests that are hardcoded in at the end of files, running your own code, etc.

I'll post the results of my own testing later this weekend.

VERY strongly suggest NO other pull requests are merged before this. Resolving merge conflicts because we let a pull request go stagnant is annoying. I take the blame on this one since I fell off the planet for a few months.

@scivision
Copy link
Author

Another choice is that this pull request is broken up into smaller pull requests. I.e. low risk, easy to test (streamlined setup.py) vs. those that go deep into the code.

@asreimer asreimer mentioned this pull request Oct 17, 2017
@scivision
Copy link
Author

Anything I can do to get this ready for AGU?

@ksterne
Copy link
Contributor

ksterne commented Dec 4, 2017

It seems as though testing on this has been slow, as it has been on much of anything davitpy. I've already created the 0.8 release branch and pull request (#327) that is set to merge in on Wednesday.

@scivision
Copy link
Author

alright probably also it's just too big a pull request addressing multiple topics. Will try to catch up with the group next week.

@asreimer
Copy link
Contributor

asreimer commented Dec 4, 2017

Quick update from me. Today I finish the draft of my thesis. After Christmas, I will finally have time to contribute again.

Testing this is going to be pretty easy. It doesn't have to be any different than any of our other testing. Since we don't have unit testing, that's the best we can do...

I suggest:

  1. Can you install davitpy?
  2. Can you still run the unit tests in radDataRead.py, etc?
  3. Run some of your own plotting/fetching codes that you have as a test both with and without this pull request.

If we get 2 or 3 people doing this and then merge it immediately after the 0.8 release, we can fix any other little bugs along the way before the next release.

I've read all the changes in this pull request. I would be surprised if this breaks anything.

@scivision
Copy link
Author

Closing this to do in several small pull requests. Thanks for your helpful input!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants