Skip to content

Conversation

pslacerda
Copy link
Member

This is my attempt to develop tests direct on the script source.

@JarrettSJohnson
Copy link
Contributor

JarrettSJohnson commented Jun 22, 2025

A bit hard to follow this PR. Changes seem to be mostly redundant with #163 . I assume that the documentation remove is orthogonal to the goals of this PR (#162), thus the changes should be removed.

I suggest creating a PR that does the script movement separately, and create a separate PR based off of those changes (to which you can rebase again when they're merged).

This PR probably also (partially) addresses issue #133 .

@speleo3
Copy link
Contributor

speleo3 commented Jun 22, 2025

Adding to what Jarrett said: I would appreciate a more comprehensive pull request description, including goals, scope, design, side-effects. E.g.:

  • Goal: Implement Testing scripts & plugins #133
  • Scope: All "scripts"
  • Out-of-scope: plugins
  • Design:
    • test_* functions are added directly to the script files like D unittest blocks.
    • All scripts moved to "scripts" because a valid Python identifier is required to PyTest collect tests (not sure). It does importing.
    • "scripts" added to PYTHONPATH so scripts can import each other (was required for "aKMT_Lys_pred.py"
  • Side-effect: All PyMOLWiki script links need updating, possibly by enhancing logic in https://pymolwiki.org/index.php/Template:Infobox_script-repo

@pslacerda
Copy link
Member Author

@JarrettSJohnson, I cleaned up the PRs, they're not redundant anymore.

Thanks @speleo3, I'll be more educated next time.

@JarrettSJohnson
Copy link
Contributor

The side-effect that Thomas mentioned is perhaps the one that I'm most concerned about. Do you have a plan for updating the links on PyMOLWiki? I assume that doing all of them at once is probably not feasible? Would it be better to just select/migrate a few representative scripts at a time?

@pslacerda
Copy link
Member Author

The reason I moved the scripts to a "package" named "scripts" was because Pytest couldn't collect all tests. In fact, only a single script crashed the test collection, and it was because of importing "findseq.py".

# From Pymol-script repo: https://pymolwiki.org/index.php/Findseq
import findseq

Maybe with pytest.importorskip it could skip the offending import. Maybe is useful to make possible scripts/modules "friends" by just importing them, but is used on the single occasion mentioned (at least for scripts, I don't analyzed plugins).

Very much sane to convert in batches sensible scripts could give room to edit any pending necessary PyMOL Wiki changes. But to edit all links in a single batch is also feasible because of https://wiki.pymol.org/index.php/Category:Pymol-script-repo, I guess.

@JarrettSJohnson
Copy link
Contributor

Ah right. I forgot/missed the part that PyMOLWiki creates these link automatically.

@speleo3 speleo3 mentioned this pull request Jun 22, 2025
@speleo3
Copy link
Contributor

speleo3 commented Jun 22, 2025

I'm not strictly against moving scripts to a "scripts" folder, certainly would improve tidiness of this repo.

But to prove that it's possible without moving files, I created #164

There is no problem with importing "findseq" from aKMT_Lys_pred.py because I added pythonpath = . to pytest.ini

@pslacerda
Copy link
Member Author

pslacerda commented Jun 22, 2025

Oh, I see, that`s right!

I'm thinking about move scripts manually one by one. What do you think? In the process, maybe I'll convert documentation examples in testable code?

And I`m unsure how to handle psico extensions like AAIndex.

@pslacerda
Copy link
Member Author

Just did the changes on master.

@pslacerda pslacerda closed this Jun 23, 2025
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.

4 participants