Repository: hyperspy/rosettasciio (language: python)

Issue:
Remove packaging of test data and download them on demand
I had a go at removing the packaging of the test file and downloading on demand when running the test suite.

Summary of the approach:
- test data live in this repository, just a different folder
- adding test data during development is similar as before
  - add the file to the git repository
  - update `registry.txt` by calling the `rsciio.tests.registry_utils.update_registry` function
- the test data are not packaged
- the test data are downloaded automatically if there are not locally available when running the test suite. It slow down running because:
  - it takes about 3 min to download everything because there are many small files and the overhead is significant.
  - running several worker is possible, which I suspect is a bug `pytest-xdist` or `pytest`

Example of the download all test data before running the test suite: https://github.com/hyperspy/rosettasciio/actions/runs/5071177296/jobs/9107256002?pr=123

### Drawback
- Running the test suite will fail, it the user doesn't write access to the test folder.
- There is an issue with generating hash on windows of text, xml file (most likely related to line ending) and it doesn't seem that we can solve it. However, we would workaround with a custom pre-commit hook that we could run from the pull request and that will be skipped on windows.

### Progress of the PR
- [x] Move test data a the `rsciio/tests/data` folder,
- [n/a] update docstring (if appropriate),
- [n/a] update user guide (if appropriate),
- [x] update contributing `CONTRIBUTING.rst` once we are happy with the approach
- [ ] Check setting base url plays well with our setup in rosettasciio to pick tagged or development branch
- [x] add an changelog entry in the `upcoming_changes` folder (see [`upcoming_changes/README.rst`](https://github.com/hyperspy/rosettasciio/blob/main/upcoming_changes/README.rst)),
- [ ] Check formatting changelog entry in the `readthedocs` doc build of this PR (link in github checks)
- [x] add tests,
- [x] ready for review.


### After the PR is merged
- [ ] Update base url from my fork to the hyperspy organisation.
- [ ] Add custom pre-commit hook to update the registry so that the registry can be updated by running pre-commit from the PR (add a comment)
- [ ] follow up with the zip file


Discussion / hints:
## [Codecov](https://app.codecov.io/gh/hyperspy/rosettasciio/pull/123?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hyperspy) Report
Patch coverage: **`87.50`**% and no project coverage change.
> Comparison is base [(`6210e4e`)](https://app.codecov.io/gh/hyperspy/rosettasciio/commit/6210e4e4daa680c6dd9fa0c67f51138a110aae1c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hyperspy) 85.24% compared to head [(`cf4486b`)](https://app.codecov.io/gh/hyperspy/rosettasciio/pull/123?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hyperspy) 85.24%.

<details><summary>Additional details and impacted files</summary>


```diff
@@           Coverage Diff           @@
##             main     #123   +/-   ##
=======================================
  Coverage   85.24%   85.24%           
=======================================
  Files          73       73           
  Lines        9034     9042    +8     
  Branches     2045     2046    +1     
=======================================
+ Hits         7701     7708    +7     
  Misses        872      872           
- Partials      461      462    +1     
```


| [Impacted Files](https://app.codecov.io/gh/hyperspy/rosettasciio/pull/123?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hyperspy) | Coverage Δ | |
|---|---|---|
| [rsciio/conftest.py](https://app.codecov.io/gh/hyperspy/rosettasciio/pull/123?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hyperspy#diff-cnNjaWlvL2NvbmZ0ZXN0LnB5) | `80.00% <87.50%> (+8.57%)` | :arrow_up: |


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/hyperspy/rosettasciio/pull/123?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hyperspy).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hyperspy).

@ericpre Very cool!  I'll probably try to check this out tomorrow to see if I can get it to work.  The `rsciio.tests.registry_utils.update_registry` function seems reasonable but I wonder if people will get stuck on that.  Is it worth it to see if we can use pre-commit to get that to check for new files and then automatically edit the registry.txt file?
> @ericpre Very cool! I'll probably try to check this out tomorrow to see if I can get it to work. The `rsciio.tests.registry_utils.update_registry` function seems reasonable but I wonder if people will get stuck on that. Is it worth it to see if we can use pre-commit to get that to check for new files and then automatically edit the registry.txt file?

Yes, this is a good idea, even if not everyone uses pre-commit, it will be useful to request pre-commit to update it from a comment in a PR.
In any case, if the registry is out of date, the `Build / Test Packaging` will fail, so it will not fall through the cracks!

Can we leave this for another PR? :)
> Yes, this is a good idea, even if not everyone uses pre-commit, it will be useful to request pre-commit to update it from a comment in a PR. In any case, if the registry is out of date, the `Build / Test Packaging` will fail, so it will not fall through the cracks!
> 
> Can we leave this for another PR? :)

Sounds good to me.  Otherwise I think it looks good. 
TODO before merging: Edit `CONTRIBUTING.rst` to update the path where test files should be placed (and if needed how to update the registry). The doc currently contains the comment *[change for pooch]* at the appropriate point.
Everyone is happy with it, merging to continue on the other PRs.

Fix patch:
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index ba9bf949b..6b40fbee2 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -3,7 +3,14 @@ repos:
     # Version can be updated by running "pre-commit autoupdate"
     rev: 23.3.0
     hooks:
-    -   id: black
+    - id: black
+  - repo: local
+    hooks:
+    - id: registry
+      additional_dependencies: ["pooch"]
+      name: Update registry
+      entry: python update_registry.py
+      language: python
 ci:
     # Don't run automatically on PRs, instead add the comment
     # "pre-commit.ci autofix" on a pull request to manually trigger auto-fixing 
diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst
index 8c4284898..dec099a2e 100644
--- a/CONTRIBUTING.rst
+++ b/CONTRIBUTING.rst
@@ -20,10 +20,11 @@ Pull Requests
 =============
 
 If you want to contribute to the RosettaSciIO source code, you can send us a
-`pull request <https://github.com/hyperspy/rosettasciio/pulls>`_. Small bug fixes
-are corrections to the user guide are typically a good starting point. But don't
-hesitate also for significant code contributions, such as support for a new
-file format - if needed, we'll help you to get the code ready to common standards.
+`pull request <https://github.com/hyperspy/rosettasciio/pulls>`_ against the ``main``
+branch. Small bug fixes are corrections to the user guide are typically a good
+starting point. But don't hesitate also for significant code contributions, such
+as support for a new file format - if needed, we'll help you to get the code ready
+to common standards.
 
 Please refer to the 
 `HyperSpy developer guide <http://hyperspy.org/hyperspy-doc/current/dev_guide/intro.html>`_
@@ -35,9 +36,7 @@ To keep the code style consistent (and more readable), `black <https://black.rea
 is used to check the code formatting. When the code doesn't comply with the expected formatting,
 the `lint <https://github.com/hyperspy/rosettasciio/actions/workflows/black.yml>`_ will fail. 
 In practise, the code formatting can be fixed by installing ``black`` and running it on the
-source code or by using `pre-commit <https://pre-commit.com>`_ to format code automatically.
-Alternatively, the comment ``pre-commit.ci autofix`` can be added to a PR to fix the formatting
-using `pre-commit.ci <https://pre-commit.ci>`_.
+source code or by using :ref:`pre-commit hooks <pre-commit-hooks>`.
 
 
 .. _adding-and-updating-test-data:
@@ -60,7 +59,10 @@ To add or update test data:
       from rsciio.tests.registry_utils import update_registry
 
       update_registry()
-    
+  
+   On windows, you can use :ref:`pre-commit.ci <pre-commit-hooks>` by adding a message to
+   the pull request to update the registry.
+
 Review
 ------
 
@@ -68,6 +70,18 @@ As quality assurance, to improve the code, and to ensure a generalized
 functionality, pull requests need to be thoroughly reviewed by at least one
 other member of the development team before being merged.
 
+.. _pre-commit-hooks:
+
+Pre-commit Hooks
+----------------
+Two pre-commit hooks are set up:
+
+* Linting: run ``black``
+* Update test data registry (Unix only)
+
+These can be run locally by using `pre-commit <https://pre-commit.com>`__.
+Alternatively, the comment ``pre-commit.ci autofix`` can be added to a PR to fix the formatting
+using `pre-commit.ci <https://pre-commit.ci>`_.
 
 .. _defining-plugins:
 
diff --git a/pyproject.toml b/pyproject.toml
index 87656dd91..ee19c12a9 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -137,6 +137,7 @@ source = ["rsciio"]
 omit = [
   "rsciio/tests/*",
   "setup.py",
+  "update_registry.py"
 ]
 
 [tool.coverage.report]
diff --git a/upcoming_changes/123.maintenance.rst b/upcoming_changes/123.maintenance.rst
new file mode 100644
index 000000000..dc2e65bb7
--- /dev/null
+++ b/upcoming_changes/123.maintenance.rst
@@ -0,0 +1,1 @@
+Remove test data from packaging and download them when necessary
\ No newline at end of file
diff --git a/upcoming_changes/129.maintenance.rst b/upcoming_changes/129.maintenance.rst
new file mode 100644
index 000000000..21ae9876e
--- /dev/null
+++ b/upcoming_changes/129.maintenance.rst
@@ -0,0 +1,1 @@
+Add pre-commit hook to update test data registry and pre-commit.ci to run from pull request
\ No newline at end of file
diff --git a/update_registry.py b/update_registry.py
new file mode 100644
index 000000000..1fd077d50
--- /dev/null
+++ b/update_registry.py
@@ -0,0 +1,17 @@
+#!/usr/bin/env python3
+import os
+import subprocess
+import sys
+
+
+def main():
+    os.chdir(r"rsciio/tests")
+    print(os.listdir())
+    cmd = ("python", "registry_utils.py", *sys.argv[1:])
+
+    return subprocess.call(cmd)
+
+
+if __name__ == "__main__":
+    # Script used by the pre-commit hook
+    exit(main())


Test patch:
diff --git a/rsciio/tests/registry.py b/rsciio/tests/registry.py
index e6916c4e1..a5d5ecfe4 100644
--- a/rsciio/tests/registry.py
+++ b/rsciio/tests/registry.py
@@ -28,9 +28,9 @@
 
 
 if Version(version).is_devrelease:
-    version = f"v{version}"
-else:
     version = "main"
+else:
+    version = f"v{version}"
 
 
 TESTS_PATH = Path(__file__).parent
@@ -38,8 +38,7 @@
 
 TEST_DATA_REGISTRY = pooch.create(
     path=TESTS_PATH / "data",
-    # base_url=f"https://github.com/hyperspy/rosettasciio/raw/{version}/rsciio/tests/data/",
-    base_url="https://github.com/ericpre/rosettasciio/raw/pooch/rsciio/tests/data/",
+    base_url=f"https://github.com/hyperspy/rosettasciio/raw/{version}/rsciio/tests/data/",
     # We don't use the version functionality of pooch because we want to use the
     # local test folder (rsciio.tests.data)
     version=None,
diff --git a/rsciio/tests/registry.txt b/rsciio/tests/registry.txt
index 99e9f1399..8d20d30bc 100644
--- a/rsciio/tests/registry.txt
+++ b/rsciio/tests/registry.txt
@@ -381,7 +381,6 @@
 'semper/example_signal_3d.unf' 794dd19387dedf9cff2e7ec3fdf46b02d11c6be945b01880cf8133ba03548435
 'semper/example_spectrum_1d.unf' 9eebdcaee503ece2b54aa21d9c50d6a25d36d550a237bdcf1261117ecc33e7dd
 'semper/example_with_title.unf' 9497c2097cd3e9756c9d4d15ea7475c27d36e699edc3e6f9780a901766fb5d44
-'test_compilers.c' 849ca8484e852e1656e0058e43417e2a0cabed35ab1691c4c1540dd7ab37c8c2
 'tia/new/128x128-TEM_search.emi' 0b0a18b5b6eeb166e0b363f5f7737003b6308f5c523b2d063c3f38bd66235f89
 'tia/new/128x128-TEM_search_1.ser' 8b79f81363d41f759ecda84fb91d50463e2a828f532d5b8f9378960e57fc3ae7
 'tia/new/128x128_TEM_acquire-sum1.emi' beff2c0d6e4a5f2d59004925697f9aac3ffbeee82943639fd15ef4f632a82885
diff --git a/rsciio/tests/registry_utils.py b/rsciio/tests/registry_utils.py
index fe280bf48..d956cd52d 100644
--- a/rsciio/tests/registry_utils.py
+++ b/rsciio/tests/registry_utils.py
@@ -18,11 +18,10 @@
 
 from pathlib import Path
 import sys
+import warnings
 
 import pooch
 
-from rsciio.tests.registry import TEST_DATA_REGISTRY
-
 PATH = Path(__file__).parent
 
 
@@ -36,7 +35,8 @@ def update_registry():
     ending.
     """
     if sys.platform == "win32":
-        raise RuntimeError("This is not supported on Windows.")
+        warnings.warn("Updating registry is not supported on Windows. Nothing done.")
+        return
 
     make_registry(
         PATH / "data",
@@ -122,6 +122,9 @@ def download_all(pooch_object=None, ignore_hash=None, progressbar=True):
         `tqdm` library. Default is True
 
     """
+
+    from rsciio.tests.registry import TEST_DATA_REGISTRY
+
     if pooch_object is None:
         pooch_object = TEST_DATA_REGISTRY
     if ignore_hash is None:
@@ -146,3 +149,8 @@ def download_all(pooch_object=None, ignore_hash=None, progressbar=True):
 
     if progressbar:
         pbar.close()
+
+
+if __name__ == "__main__":
+    # Used by the pre-commit hook
+    update_registry()