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

Fix formatting ofr README.rst #175

Merged
merged 3 commits into from
May 17, 2024
Merged

Fix formatting ofr README.rst #175

merged 3 commits into from
May 17, 2024

Conversation

hamogu
Copy link
Contributor

@hamogu hamogu commented May 12, 2024

Python examples are now rendered a code blocks.

Copy link

codecov bot commented May 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (9ba69b4) to head (339cd27).

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #175   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           44        44           
  Lines         2129      2129           
=========================================
  Hits          2129      2129           
Flag Coverage Δ
3.10 100.00% <ø> (ø)
3.11 100.00% <ø> (ø)
3.12 100.00% <ø> (ø)
3.8 100.00% <ø> (ø)
3.9 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jagerber48
Copy link
Owner

@hamogu thanks for your contribution. It seems something changed with github a few months ago that made the readme.rst stop rendering properly on the github main page for the repo. This is despite the facts that the readme.rst used to render appropriately there and it still renders fine on the documentation at read the docs and on pypi. This must be related to these issues affecting many users: https://github.com/orgs/community/discussions/86715.

It look like your changes fix the code block rendering on github which is very helpful. Do you have any ideas what needs to be done to correct the spacing of the badges and emphasis in the first section?

compare good (readthedocs):
image
to bad (github):
image

README.rst Outdated Show resolved Hide resolved
@hamogu
Copy link
Contributor Author

hamogu commented May 13, 2024

This look OK on GH (I'm editing the file on the web so that I can use the "preview" button - presumably that's giving me the same layout that it will display later. However, need to check that it also renders correctly in normal Sphinx. It often does - GH markup is more restricted - but the only way to check is to actually run it and it look good there, too:

Screenshot 2024-05-13 at 9 58 04 AM

Python examples are now rendered a code blocks in github as well as in sphinx.
@hamogu
Copy link
Contributor Author

hamogu commented May 13, 2024

Rebased to squash into to logical commits.

| **PyPi:** `<https://pypi.org/project/sciform/>`_
| **Repository:** `<https://github.com/jagerber48/sciform>`_
| **Documentation:** `<https://sciform.readthedocs.io/en/stable/>`_
| **PyPi:** `<https://pypi.org/project/sciform/>`_

Copy link
Owner

Choose a reason for hiding this comment

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

Could you please have a blank line appear in the rendered output between the PyPi line and the "We would greatly appreciate..." line?

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 wish I could, but it seems that GH does not render it the way I think it should. I tried an extra | attached or in it's own block, I tried empty lines, etc but GH does not render either. I also don't want to get too complicated just just a blank line in a readme. I think it looks better already and I can't find an easy solution to add a blank line. I suggest to leave as is and live with it - perfect is the enemy of good - and rather put our time into more important things.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah ok, No problem then. Didn't realize GH is doing something to make it challenging to include a blank line! Ok, I'll look over everything more carefully when I get a chance!

@jagerber48
Copy link
Owner

Yes, it looks like it's all rendering great on your fork: https://github.com/hamogu/sciform/tree/patch-1

Thank you so much! This has been bothering me for a while now but I haven't known how to fix it. The tip that github markdown is more restrictive than e.g. sphinx or only rst renderers is annoying but helpful.

When I get more time this week I'll take a look through everything and move this forward.

@jagerber48
Copy link
Owner

@hamogu I would like to merge this PR but I'd like your help with something. So far all PRs into sciform have been authored by me. I've enacted a personal policy of including changelog entries on all PRs that warrant them, but this ends up warranting me doing a few bits of specific formatting in the changelog file.

I haven't yet come up with what the general workflow should be for PRs that aren't authored by me. I don't want to burden future PRs with finnicky details about formatting of the changelog.

Do you have any suggestion for me?

One idea I have, at least for this PR, is that we can just forget about the changelog and I can open up a follow-up PR with the changelog information. The other idea is to use a changelog tool like towncrier or something but it would take me some time to learn that.

@hamogu
Copy link
Contributor Author

hamogu commented May 17, 2024

You are the author and get to choose...

However, I typically would not do a changelog for something like this at all. Nothing in the code changed. To me, the main purpose of a changelog is to see "could anything brake in my code if I update my version of sciform?". Changes that are docs only, or typos in comments in the code etc. can't do that, so I'd leave them out.
Of course, some projects like to use Changelogs also to give credit to contributors.

I also think it's not too much to ask me to put in a changelog, if you want one. Alternatively, you can tell me that you are going to do that either do it after (as you suggested) or push it to my branch so it becomes part of this PR before you merge: The default in github is now to allow maintainers edit PRs by pushing to branches in a PR - and if you look at the top right of this page, you'll see that the tick-mark is set for "allow edits by maintainers", which means that you have write access to my branch.

I don't think towncrier is worth it for a project of this size. We use it in e.g. astropy, but there we have literally dozens of people contributing to every release and dozens of PRs open at the same time, so that you often get merge conflicts in the changelog file because everybody edits it at the same location. For sciform, that's not that case. The project is much more focussed on one particular issue thus much smaller, so I think a more complex tool like towncrier (or similar) is a waste of time when a much simpler way (just edit a file) with no extra dependencies also does the job - instead of adding more tools that are not needed, it's probably better to spend the time on fixing actual issues or working on something else!

@jagerber48
Copy link
Owner

Thank you so much for taking the time to share that information and feedback to educate me. I was wanting more ideas for different options and norms and you gave me exactly that.

Yes, I realize many projects wouldn't include docs fixes in the changelog. So far I've let the changelog be quite verbose. This makes it a bit of a development log rather than just a "need-to-know" log for users. Perhaps as time goes on I'll become more comfortable with just letting the PR history serve as the development log and the changelog can become more concise.

Ok, good to know that I can push to branches in downstream forks since I'm an upstream maintainer. That would be an easy way for me to do this in most cases. Especially, of course, possible future cases that more obviously warrant an entry in the changelog.

Also good to hear that towncrier is a waste of effort for a small project. I think that was my sense also, so it's good to have that confirmed.


Ok, for this PR I'll just go ahead and merge it and will ponder on my own time whether I want to follow up with a changelog entry for this fix. Thanks again for your contribution!

@jagerber48 jagerber48 merged commit 2b4d620 into jagerber48:main May 17, 2024
7 checks passed
@hamogu hamogu deleted the patch-1 branch May 17, 2024 15:56
@hamogu
Copy link
Contributor Author

hamogu commented May 17, 2024

No problem, I'm glad if it's helpful. Those would also be good discussions to have on e.g. the pyopenSci slack where a lot of people discuss the tools they use for their projects. If you use slack, you might find it interesting to join that and just see what others are doing. Maintainers of packages listed in PyOpenSci usually get invited during the review, not sure if that happened in your case (pyOpenSci/software-submission#121) but if not, I'm happy to do that now.

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