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

Fixed wrong tox targets #260

Merged
merged 2 commits into from
Apr 8, 2021
Merged

Fixed wrong tox targets #260

merged 2 commits into from
Apr 8, 2021

Conversation

andamian
Copy link
Contributor

@andamian andamian commented Apr 8, 2021

@codecov
Copy link

codecov bot commented Apr 8, 2021

Codecov Report

Merging #260 (ddea53b) into master (062710b) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #260   +/-   ##
=======================================
  Coverage   74.83%   74.83%           
=======================================
  Files          43       43           
  Lines        5035     5035           
=======================================
  Hits         3768     3768           
  Misses       1267     1267           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 062710b...ddea53b. Read the comment docs.

@andamian
Copy link
Contributor Author

andamian commented Apr 8, 2021

@pllim or @bsipocz - do you have a minute to take a quick look at this? It's quite simple and straightforward but if you have a better idea please let me know. Thanks

run: tox -e py${{ matrix.python-version }}-${{ matrix.astropy-version }}
run: |
# remove "." from matrix.python-version to get the defaults tox python versions
tox_python_target=$(echo py${{matrix.python-version}} | sed 's/\.//')
Copy link
Member

Choose a reason for hiding this comment

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

I think it is less confusing to set tox_python_target as another parameter for the matrix above, rather then doing some sed calls. This won't work if you want to inject other OS in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I understand what you mean. Add another dimension to the matrix and only run if python-version matches the tox_python_target?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is some package that enables explicit mapping of GH python version with the tox target but it doesn't seem worth it.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking something like https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#example-including-additional-values-into-combinations but now that I look at it again, I realize maybe that's not what you want.

If this syntax works for you for now, I guess it is okay to deploy as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does the trick for now. If it breaks with future version then will address it at that time I guess. Thanks for your quick feedback!

@andamian andamian merged commit 767a859 into astropy:master Apr 8, 2021
@andamian andamian deleted the actions branch April 8, 2021 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants