skpkg: setup CI after migrating tests, src, requirements, and .github folder #20

stevenhua0320 · 2025-10-03T00:47:10Z

@zmx27 Ready for review, I'm in the step of migration of files

zmx27

See comments

zmx27 · 2025-10-03T16:20:03Z

.github/workflows/tests-on-pr.yml

+      c_extension: false
+      headless: false
+    secrets:
+      CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}


There's a workflow file in our release-scripts repo that's similar to the _tests-on-pr.yml workflow script except it doesn't upload to Codecov. I believe that it's best to call upon that instead. So you'd call it like:

uses: scikit-package/release-scripts/.github/workflows/_tests-on-pr-no-codecov.yml@v0

The reason we do this is that this is a private repo, and uploading to Codecov would not work. Then, delete the line containing the Codecov token. Similarly, we want our CI to run pre-commit in every PR, so I would add a section with user run commands as well. So the final segment would be something like:

jobs: tests-on-pr: uses: scikit-package/release-scripts/.github/workflows/_tests-on-pr-no-codecov.yml@v0 with: project: diffpy.pdfgetx c_extension: false headless: false run: | conda install pre-commit pre-commit run --all-files

So for here, is that project: diffpy.pdfgetx to be replaced by project: diffpy.srxplanargui?

Yes, my bad. I happened to have a tab open for pdfgetx and just copy pasted a section of it haha

zmx27 · 2025-10-03T16:23:46Z

pyproject.toml

 namespaces = false  # to disable scanning PEP 420 namespaces (true by default)

+[project.scripts]
+diffpy-srxplanargui = "diffpy.srxplanargui.app:main"


I don't see such a file in the project repo

It is uploaded in one of the commits, there is a file called diffpy.srxplanargui_app.py

I assume it would be called as "diffpy.srxplanargui_app:main"

Have changed to you said, see new commit.

zmx27 · 2025-10-03T16:25:06Z

pyproject.toml

  | build
  | dist
-
+  | dpx/srxplanargui/(live|selectfiles|imageplot)\.py


Can I ask why these files are being excluded? And why dpx?

These files are excluded because as I said in the linting issue before, there is pre-commit conflict between flake8 and isort auto-fix, so in order to suppress that, I excluded these files. Originally it is in dpx (my bad, I forgot to delete that in new version)

You won't need to exclude these files if you already # noqa those lines. That will already suppress the flake8 errors, and you can just default to the isort autofixes

Sorry, my bad. It is not flake8 issue but black and isort formatting conflict

zmx27 · 2025-10-03T16:25:27Z

pyproject.toml

+skip = [
+  "src/diffpy/srxplanargui/live.py",
+  "src/diffpy/srxplanargui/selectfiles.py",
+  "src/diffpy/srxplanargui/imageplot.py",


Same here, why are these being skipped?

Same reason as previous one

Now that I think about it, why is this here? .pre-commit-config.yaml already sets the "profile" of isort to black, and you can also exclude certain files in that config file directly (if it comes to that in the worst case). This feels kind of unnecessary

I have deleted the hook exclusion files for black . Now it seems to be right on my local. Let's try not to exclude files for black check and also edits in .pre-commit-config.yaml. So, now these exclusion for isort is necessary. Also, I made these exclusion to .isort.cfg file so it would be deleted in the latest commit.

zmx27 · 2025-10-03T16:30:58Z

requirements/conda.txt

+chaco
+fabio
+pyfai
+matplotlib


According to the old pip.txt, it seems like pyface is also a dependency?

zmx27 · 2025-10-03T16:31:38Z

requirements/pip.txt

-pyface
-diffpy.srxplanar
-configparser
+numpy


Why did you delete all of this? configparser can be left deleted, however, since it is now in the Python standard library as of Python 3.

Because I assumed that diffpy.srxplanar would perferrably be installed via conda since we prefer more stable build of dependencies. (My bad, I forgot to push that commit for this edition, I would do that shortly.)

This is a private repo, and I don't believe we intend to upload this to conda forge and allow it to be conda installed

zmx27 · 2025-10-03T16:31:57Z

src/diffpy/srxplanargui/functions.py

@@ -0,0 +1,31 @@
+import numpy as np


Delete this file, it's a template file created by skpkg

Have deleted, see new update

tests/test_functions.py

… dependency, delete useless linting exclusion file command

zmx27

See comments

zmx27 · 2025-10-03T18:31:44Z

.pre-commit-config.yaml

+    rev: 24.4.2
+    hooks:
+      - id: black
+        exclude: '(^|/)(diffpy/srxplanargui)/(live|selectfiles|imageplot)\.py$'


You mentioned linting issues between flake8 and isort, so why are these files excluded for black as well?

Sorry, not flake8, it is black and isort autofix conflict as in #3

Hmm, I don't know why that is happening. First, try adding in the extra config file .isort.cfg. Maybe also use this chance to add in .flake8 and .codespell/. If the auto fix conflict still comes up (uncomment these excludes to test), you can probably just leave it there and ask Professor Simon whether it's okay. Because as far as I know, it's not a very good idea to allow black to ignore multiple files.

zmx27 · 2025-10-03T18:33:05Z

requirements/pip.txt

-pyface
-diffpy.srxplanar
-configparser
+pyface


What about those other dependencies that you deleted?

traits and traitsui have conda version as I checked, so I add those to conda.txt

For pyface, I have added to conda.txt and for configparser you just said we could left deleted.

zmx27 · 2025-10-03T20:12:55Z

.pre-commit-config.yaml

+    rev: 24.4.2
+    hooks:
+      - id: black
+        exclude: '(^|/)(diffpy/srxplanargui)/(live|selectfiles|imageplot)\.py$'


Hmm, I don't know why that is happening. First, try adding in the extra config file .isort.cfg. Maybe also use this chance to add in .flake8 and .codespell/. If the auto fix conflict still comes up (uncomment these excludes to test), you can probably just leave it there and ask Professor Simon whether it's okay. Because as far as I know, it's not a very good idea to allow black to ignore multiple files.

zmx27 · 2025-10-03T20:18:08Z

pyproject.toml

+skip = [
+  "src/diffpy/srxplanargui/live.py",
+  "src/diffpy/srxplanargui/selectfiles.py",
+  "src/diffpy/srxplanargui/imageplot.py",


Now that I think about it, why is this here? .pre-commit-config.yaml already sets the "profile" of isort to black, and you can also exclude certain files in that config file directly (if it comes to that in the worst case). This feels kind of unnecessary

zmx27 · 2025-10-03T20:25:21Z

requirements/pip.txt

 pyface
 diffpy.srxplanar
-configparser
+diffpy.srxconfutils


This package won't be in PyPI I believe.
Also, what I meant is that I believe the dependencies listed in pip.txt and conda.txt should be more or less the same except for when there are certain exceptions such as when packages are only on conda forge and not PyPI or when the package names are slightly different. So dependencies like traits, scipy, etc should also be here

But for diffpy.srxconfutils it is a dependency for diffpy.srxplanrgui, so where should it be as we need it.

zmx27

I haven't checked all the files yet, but I will say that this PR is becoming too big and complicated. For good practice, we usually want each PR to be quite small and lightweight and divide up the skpkging work into multiple PRs instead of doing everything at the same time. I'd suggest referring back to the skpkg documentation here to see what you want to include in this step of the skpkging process. For example, let's only add the tests-on-pr.yml file in the .github folder for now. We also usually want to add the config files for the precommit hooks in another PR. I initially mentioned adding them as a possible solution to the auto fix conflict you were encountering, but since you ended up creating some auto fixes now that black is not skipped anymore, it's probably fine to keep both the config files (like .flake8, .isort, etc) and the auto fixes. The rest, like README.rst and MANIFEST.in should be removed from this PR.

zmx27 · 2025-10-05T23:17:09Z

.github/workflows/build-wheel-release-upload.yml

@@ -0,0 +1,18 @@
+name: Release (GitHub/PyPI) and Deploy Docs


This file will change because I don't think we're releasing this package to PyPI, etc. I suggest we work on this in another PR.

zmx27 · 2025-10-05T23:17:45Z

.github/workflows/matrix-and-codecov-on-merge-to-main.yml

@@ -0,0 +1,21 @@
+name: CI


Same for this file

zmx27 · 2025-10-05T23:17:53Z

.github/workflows/publish-docs-on-release.yml

@@ -0,0 +1,12 @@
+name: Deploy Documentation on Release


Same for this

zmx27 · 2025-10-05T23:22:58Z

.isort.cfg

+multi_line_output = 3
+include_trailing_comma = True
+profile=black
+skip=src/diffpy/srxplanargui/live.py,src/diffpy/srxplanargui/selectfiles.py,src/diffpy/srxplanargui/imageplot.py


Does this mean that you need the skip to avoid the auto fix conflicts?

I have retest this on a clean branch and found that now we do not need to skip these files.

zmx27 · 2025-10-05T23:26:18Z

requirements/build.txt

This file can be deleted because this package only has Python code

stevenhua0320 · 2025-10-06T01:06:00Z

@zmx27 I would close this PR because it is too messy to make more commits. Instead I would create a new clean branch to redo pieces of work into multiple PRs so that it could be reviewed easily.

stevenhua0320 added 5 commits October 2, 2025 20:21

skpkg: add tests folder

9ab18c2

skpkg: list dependencies in requirements folder

2ca745d

skpkg: add CI and issue/PR templates

ff68e62

skpkg: add pyproject.toml

fbbbd6a

skpkg: migrate left files in src folder

3af03da

stevenhua0320 requested a review from zmx27 October 3, 2025 00:49

zmx27 reviewed Oct 3, 2025

View reviewed changes

stevenhua0320 added 2 commits October 3, 2025 13:56

skpkg: change workflow to private compatible, add missing requirement…

b08062a

… dependency, delete useless linting exclusion file command

skpkg: delete template file

ba7f3de

stevenhua0320 requested a review from zmx27 October 3, 2025 18:02

zmx27 reviewed Oct 3, 2025

View reviewed changes

skpkg: change pip and conda install dependency for correct install

744e0a3

stevenhua0320 requested a review from zmx27 October 3, 2025 19:00

zmx27 reviewed Oct 3, 2025

View reviewed changes

stevenhua0320 added 2 commits October 3, 2025 17:09

skpkg: add config files for pre-commit readthedocs, codecov, GitHub

62a0774

skpkg: change dependency for pip and conda

b1ac8e0

stevenhua0320 requested a review from zmx27 October 3, 2025 21:20

skpkg: pre-commit auto-fix

d7373d7

zmx27 reviewed Oct 5, 2025

View reviewed changes

stevenhua0320 closed this Oct 6, 2025

skpkg: setup CI after migrating tests, src, requirements, and .github folder #20

skpkg: setup CI after migrating tests, src, requirements, and .github folder #20

Uh oh!

Conversation

stevenhua0320 commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zmx27 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zmx27 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zmx27 left a comment

Choose a reason for hiding this comment

Uh oh!

stevenhua0320 commented Oct 3, 2025 •

edited

Loading

zmx27 left a comment •

edited

Loading