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 README.rst in post-release #2037

Merged
merged 8 commits into from
Jun 2, 2022

Conversation

epassaro
Copy link
Member

@epassaro epassaro commented May 27, 2022

📝 Description

Type: 🚀 feature | 🎢 infrastructure

I think this PR should not be merged. Why? Well, we want to keep a copy of the credits section in README.rst, but GitHub does not render the include directive for security reasons. This implies:

a) keep duplicated RST code and update it manually
b) this complicated pull request to automate the rendering

Shouldn't be easier not having the full citation in the readme and just using a link to redirect users to the appropriate documentation page?


Changes proposed in this pull request:

  • Create zenodo.rst on post-release workflow, outside Sphinx's conf.py
  • Update credits.rst via the rst-include package and credits_template.rst
  • Update README.rst via the rst-include package and README_TEMPLATE.rst
  • Push credits.rst and README.rst via the post-release pull request

📌 Resources

Examples, notebooks, and links to useful references.

🚦 Testing

How did you test these changes?

  • Testing pipeline
  • Other method (describe)
  • My changes can't be tested (explain why)

Not tested yet.

☑️ Checklist

  • I requested two reviewers for this pull request
  • I updated the documentation according to my changes
  • I built the documentation by applying the build_docs label

Note: If you are not allowed to perform any of these actions, ping (@) a contributor.

@epassaro epassaro marked this pull request as draft May 27, 2022 16:03
@codecov
Copy link

codecov bot commented May 27, 2022

Codecov Report

Merging #2037 (eb35258) into master (b237d51) will not change coverage.
The diff coverage is n/a.

❗ Current head eb35258 differs from pull request most recent head 6d9ebe9. Consider uploading reports for the commit 6d9ebe9 to get more accurate results

@@           Coverage Diff           @@
##           master    #2037   +/-   ##
=======================================
  Coverage   59.87%   59.87%           
=======================================
  Files          70       70           
  Lines        8157     8157           
=======================================
  Hits         4884     4884           
  Misses       3273     3273           

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@epassaro epassaro marked this pull request as ready for review May 27, 2022 16:34
@andrewfullard
Copy link
Contributor

So am I correct in assuming that this PR is to show that this would be an over-complex way to solve the problem, and you would rather link to the docs in the readme?

@epassaro
Copy link
Member Author

So am I correct in assuming that this PR is to show that this would be an over-complex way to solve the problem, and you would rather link to the docs in the readme?

That's my opinion, but it's up to you guts. I think @wkerzendorf is interested on having the complete citation in both places and I don't think there's an easier way to do this.

If that's ok for you, then let me review the changes in this PR once again, and can be merged.

@andrewfullard
Copy link
Contributor

I will leave @wkerzendorf to decide on this.

Copy link
Member

@wkerzendorf wkerzendorf 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 to me

@epassaro
Copy link
Member Author

epassaro commented Jun 2, 2022

Ok, I'll review it again and then we can merge

@epassaro epassaro enabled auto-merge (squash) June 2, 2022 16:25
@epassaro epassaro merged commit a1375f9 into tardis-sn:master Jun 2, 2022
epassaro added a commit to epassaro/tardis that referenced this pull request Jun 2, 2022
* Fix docs badge

* Add license badge

* First attempt of creating credits.rst and README.rst via templates

* Working version

* Restore old README.rst

* Update post-release workflow

* Change .gitignore

* Various fixes
epassaro added a commit to epassaro/tardis that referenced this pull request Jun 2, 2022
* Fix bot aliases in .mailmap (tardis-sn#2038)

* Added v_inner and v_outer, enable_nonhomologous_expansion for future use in nonhomologous expansion (tardis-sn#2024)

* added v_outer and v_inner to NumbaModel

* defined default value for v_inner and v_outer in NumbaModel

* added enable_nonhomologous_expansion to io/schemas/montecarlo.yml

* added v_inner and v_outer to NumbaPlasma

* added v_inner and v_outer as arguments for NumbaModel in run() in montecarlo_numba

* added v_outer_cgs to MonteCarloRunner

* removed a comment

* ran black on the changed files

* added right arguments in test

* added v_inner, v_outer to tests

* added temporary fix to formal_integral.py

* ran black on modified files

* changed dummy values in the test

* Reverted content of file.

* made needed change in test_cuda_formal_integral.py

* made needed change in test_numba_formal_integral.py

* Update README.rst in post-release (tardis-sn#2037)

* Fix docs badge

* Add license badge

* First attempt of creating credits.rst and README.rst via templates

* Working version

* Restore old README.rst

* Update post-release workflow

* Change .gitignore

* Various fixes

* Check PEP8 on CI (tardis-sn#2039)

* Add Flake8 job

* Change to file description

* Test payload

Co-authored-by: Sona Chitchyan <[email protected]>
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.

3 participants