Skip to content

Conversation

@gregstarr
Copy link
Contributor

Same as #27 except between develop branches

@gregstarr
Copy link
Contributor Author

Looks like all the CI tests failed, but I was able to get most of the tests to pass in windows subsystem for linux

Added some missing docstrings an simplified some tests.
Made a handful of PEP8 improvemeents and added a missing docstring.
Fixed unit tests that were failing on Travis due to a change in warning behaviour.  Also added some missing docstrings and added more constraints to the unit tests under scrutiny.
Reverted style include whitespace around code pertaining to a block comment and ajusted line length.
Future-proofed the new doctrings.
Updated the changelog with a summary of updates in this pull request.
@aburrell
Copy link
Owner

aburrell commented Feb 2, 2021

Ok @gregstarr this looks pretty good. I made some changes:

  1. fixed the failing unit tests not related to your code. I expect Travis to pass, since I was able to get the same tests to fail locally and fix them.
  2. made a few style changes. Obviously the contributing docs need to be improved to assist future contributions, but there's a lot here I need to get on!
  3. updated the changelog

Anyway, the core elements of your contribution look great and the additional unit tests are appropriate. Before approving I need:

  • Travis to pass (appveyor is failing due to changes in the appveyor system, and you tested windows)
  • you to add your name to the end of AUTHORS.rst
  • you to update the subsol docstring so that the Parameters description reflects the new allowable input.

@gregstarr
Copy link
Contributor Author

gregstarr commented Feb 3, 2021

I added to the subsol docstring and to AUTHORS.rst but I can't get the tests to pass on Windows 10. Basically all tests fail with the error: AttributeError: function 'cofrm_' not found. It looks like a compilation problem because the compiled file '.conda\\envs\\apexpy\\lib\\site-packages\\apexpy-1.0.3-py3.8-win-amd64.egg\\apexpy\\fortranapex.cp38-win_amd64.pyd' does not have cofrm_ in it. I will take a look, but I don't have much experience with this type of thing.

@aburrell
Copy link
Owner

aburrell commented Feb 3, 2021

Ok, since that problem is not related to this PR, I am going to merge it in.

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