Fixing contributing.rst access on windows systems

[roche-emmanuel]

This is a simple pull request to validate the changes automatically introduced with the precommit checks and fix the readthedocs automatic build accordingly.

• Add your name to AUTHORS.md if not there already

[coveralls]

Pull Request Test Coverage Report for Build 10602559011

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 96.151%

---

Totals
Change from base Build 10528275069:	0.0%
Covered Lines:	52408
Relevant Lines:	54506

---

💛 - Coveralls

[roche-emmanuel]

Hi everyone,

In this PR, I have mainly focused on modifying the doc/source/dev_guide/CONTRIBUTING.rst file. Initially, this was a simple symlink pointing to ../../../CONTRIBUTING.rst. However, when checking out the Satpy repo on a Windows machine, this symlink gets converted into a file containing only ../../../CONTRIBUTING.rst.

At this point, if you try to run the pre-commit checks, the end-of-file checker will add a \n at the end of that "file," and you might then commit that change to the repo. However, for Git, this is still treated as a symlink, and it will append the \n character to the link source path (at least, that has been my experience).

This behavior confuses the ReadTheDocs build workflow entirely, likely because it runs on Linux and attempts to read a link source such as ../../../CONTRIBUTING.rst\n.

The solution I’ve implemented here is to avoid using a symlink altogether and instead rely on the RST include directive to load the ../../../CONTRIBUTING.rst file.

With this change, the ReadTheDocs build seems to work fine, and access to the CONTRIBUTING.rst file also appears to be correct based on the workflow results on this page: https://satpy--2890.org.readthedocs.build/en/2890/dev_guide/index.html (clicking on the "How to contribute" links).

So, I’m now marking this PR as ready for review. Of course, please let me know if you think there’s anything that should be changed before it can be merged. Thanks 🙏!

[djhoese]

Thanks for this effort. And thanks for letting us know this causes issues on Windows. I have another suggestion based on something I've seen in other projects and something I've done in my own projects. What if we move the contents of /CONTRIBUTING.rst to the doc/source/dev_guide/CONTRIBUTING.rst file? Then replace the contents of /CONTRIBUTING.rst with a simple sentence like "See for information on contributing".

This has a couple benefits:

1. Everything rendered in the docs is in the docs/ sub-directory.
2. We could potentially use all the rendering tricks provided by sphinx and our configuration of our sphinx documentation. The user should typically only see the final result on the readthedocs website.

Thoughts?

[roche-emmanuel]

Hi @djhoese ,

Thanks for your review and feedback on this PR.

Concerning your request for change above, I also think it's a good idea to keep all the sources for the doc generation self-contained in the doc/ folder.

Now, from a "github perspective", I think the /CONTRIBUTING.rst file is the most important one; and this is the target that will be used at different locations on github, for instance just at the bottom of this page ;-). And I would say that "contribution guidelines" are most of the time used by developers while doing some work: so there is also a good chance this /CONTRIBUTING.rst file will be accessed regularly for reference. That's why I would suggest providing an actual link to the rendered guidelines in this file, so something like that in the end:

Contributing Guidelines
=======================

For detailed contribution guidelines, please see our `Developer's Guide on ReadTheDocs <https://satpy.readthedocs.io/en/stable/dev_guide/index.html>`_.


.. If you're reading this file locally as a plain text, you may also
   directly refer to the file doc/source/dev_guide/CONTRIBUTING.rst for
   any unmerged/pending changes to the contribution guidelines.

Note: I also considered using the rst include statement there (ie. .. include:: doc/source/dev_guide/CONTRIBUTING.rst) but unfortunately, this rst command is ignored by the github preview, so this would not really help in a development context [plus, it would still not use any advanced rendering trick as you mentioned above]

=> I have prepared another test branch on this, updating the two CONTRIBUTING.rst files accordingly (https://github.com/roche-emmanuel/satpy/tree/moving_contributing_rst). If this sounds OK to you then I can proceed with merging that other branch into this one (Or should I wait for the review from Martin before introducing more changes on this PR anyway ? Please let me know in this case)

[mraspaud]

@roche-emmanuel you definitely don't have to wait for me to continue :)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.05%. Comparing base (5e27be4) to head (9215322).
Report is 76 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2890   +/-   ##
=======================================
  Coverage   96.05%   96.05%           
=======================================
  Files         370      370           
  Lines       54320    54320           
=======================================
  Hits        52177    52177           
  Misses       2143     2143           

Flag	Coverage Δ
behaviourtests	3.99% <ø> (ø)
unittests	96.15% <ø> (ø)

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.

[roche-emmanuel]

Hi @djhoese ,

I have now merged the changes discussed above on this branch. Please let me know if you think this should be done differently when you get a chance to review this update. Thanks 😉🙏!

[djhoese]

I believe the README link was created a long time ago when either we assumed that GitHub didn't find a README with a filename extension or it didn't actually work so we needed it. It doesn't look necessary anymore so I'm good with the removal.

[djhoese]

Looks good to me. @mraspaud any last comments before this is merged?

[mraspaud]

LGTM!