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 CI including sphinx for documentation #124

Merged
merged 11 commits into from
Oct 29, 2021
Merged

Conversation

campanelli-sunpower
Copy link
Contributor

@campanelli-sunpower campanelli-sunpower commented Oct 28, 2021

@kanderso-nrel pointed out some issues in #122 with CircleCI builds, which are addressed here. I also hardened and cleaned up the builds, including better reporting of the test results/artifacts.

Followup work will be reviewing/improving automation on tagged releases, including Github artifacts, documentation, and pypi publishing.

- ~/.bundle
- ~/.go_workspace
- ~/.gradle
- ~/.cache/bower
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This cache is not needed for python. In particular, no virtualenv used here.

@@ -34,7 +34,7 @@ class ExampleReportBuilder(object):
def build(pvarray):
"""Method that will build the simulation report. Here we're using the
previously defined
:py:function:`~pvfactors.report.example_fn_build_report`.
:py:func:`~pvfactors.report.example_fn_build_report`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This typo generates a warning, but is not actually included in the doc'n.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this method should be included in the docs? Not sure if that would be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to have link in first sentence, which is the only one Sphinx includes in the doc'n.

@@ -265,7 +265,7 @@ def _run_serially(args):
----------
args : tuple
List of arguments where most will be used in
:py:function:`~pvfactors.run.run_timeseries_engine`
:py:func:`~pvfactors.run.run_timeseries_engine`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This typo generates a warning, but is not actually included in the doc'n.

@campanelli-sunpower
Copy link
Contributor Author

@kanderso-nrel I couldn't add you as an official reviewer, but I would appreciate your informal review. Thanks.

@campanelli-sunpower
Copy link
Contributor Author

campanelli-sunpower commented Oct 28, 2021

Moving this PR to a fork, so it can be visible to the open source community.

I think I was mistaken about visibility here. Reopening.

Copy link
Contributor

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

I'm not a circle-ci user but LGTM, a couple minor notes below

- ~/.gradle
- ~/.cache/bower
- run: pytest
- run: pytest --junitxml=$CIRCLE_TEST_REPORTS/junit.xml
- store_test_results:
path: /tmp/circleci-test-results
- store_artifacts:
path: /tmp/circleci-artifacts
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal, but I think this entry might not be doing anything:

Uploading /tmp/circleci-artifacts to tmp/circleci-artifacts
  No artifact files found at /tmp/circleci-artifacts
Total size uploaded: 0 B

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed this and left it in for now because I'm not sure if something is supposed to be saved there, say for uploads to pypi. Sorry that I didn't add a comment.

@@ -34,7 +34,7 @@ class ExampleReportBuilder(object):
def build(pvarray):
"""Method that will build the simulation report. Here we're using the
previously defined
:py:function:`~pvfactors.report.example_fn_build_report`.
:py:func:`~pvfactors.report.example_fn_build_report`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this method should be included in the docs? Not sure if that would be helpful.

Copy link

@rdesaisp rdesaisp left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@campanelli-sunpower campanelli-sunpower merged commit acfb7e2 into master Oct 29, 2021
@campanelli-sunpower campanelli-sunpower deleted the update_sphinx branch October 29, 2021 19:53
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.

3 participants