Skip to content

Add AAAS journals to subplot command #30

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

Merged
merged 2 commits into from
Sep 6, 2019
Merged

Add AAAS journals to subplot command #30

merged 2 commits into from
Sep 6, 2019

Conversation

bradyrx
Copy link
Collaborator

@bradyrx bradyrx commented Sep 4, 2019

@lukelbd, this PR adds the AAAS journal specs to the proplot.subplots() command. It also has a few minor fixes:

  • Move the journal specs table to the header and change name to JOURNAL_SPECS following PEP (https://www.python.org/dev/peps/pep-0008/#constants).
  • Fix try/except clause in subplots() that was causing certain journal specs to break.
  • Add pytest for all journal specs as a template for future testing.
  • Update changelog and add capability in docs to link from changelog to PR/issue threads.

This is a great simple example of pytest's power. I created the test then ran pytest and found consistent errors in certain subsets of the dictionary (TypeError vs. ValueError). Once you get pytest implemented into Travis CI, this will run automatically on PRs and then you have no risk of importing broken code. At least broken journal spec code for now.

@bradyrx
Copy link
Collaborator Author

bradyrx commented Sep 4, 2019

Feel free to review directly if you want anything changed. Or you can pull it down and edit, push, then merge. You need to add a travis CI webhook, and .travis file, etc. for the tests to run on a PR like this.

@bradyrx
Copy link
Collaborator Author

bradyrx commented Sep 4, 2019

FYI, this is a solid template for travis: https://github.com/bradyrx/climpred/blob/master/.travis.yml. I may be biased though.

Then you just need to activate your repo here (https://travis-ci.org/) and add the webhook. Then it will always run on every PR, ensuring that the package can be built and that all testing passes.

@lukelbd
Copy link
Collaborator

lukelbd commented Sep 6, 2019

Looks good to go. Thanks for the tips. Didn't know about the PEP8 recommendation re: constants. I have a bunch of constants throughout the module, decided to also make them all caps (c2ae600) and gotta admit it's nice to know at-a-glance where each function is using a global variable.

@lukelbd lukelbd merged commit 43943a7 into proplot-dev:master Sep 6, 2019
@bradyrx
Copy link
Collaborator Author

bradyrx commented Sep 6, 2019

Didn't know about the PEP8 recommendation re: constants.

These are the kind of things that come up through open source code review via PRs. I learned that one from a collaborator on climpred.

FYI, @lukelbd, the ext_links feature allows for this (clickable PR links in a changelog): https://climpred.readthedocs.io/en/latest/changelog.html

So eventually you might want to host that on your docs.

@bradyrx bradyrx deleted the add_aaas_journals branch September 6, 2019 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants