Improvements on the `@cmd.declare_command` API #448

pslacerda · 2025-04-12T07:51:07Z

A write a bunch of tests and did my best to achieve best ergonomic API.

Only optional (None) data is still failing in some cases.

JarrettSJohnson · 2025-05-12T23:09:14Z

I'll ask if we can maintain our pattern of keeping tests in testing (rather than adding tests in module).

pslacerda · 2025-05-13T00:08:45Z

Sorry, it was my bad. I'll tackle this in a few days.

pslacerda · 2025-05-13T15:48:34Z

@JarrettSJohnson I'd like to ask, should I proceed and refactor existing commands to adhere the declare_command API?

pslacerda · 2025-05-13T15:52:07Z

The first step is to refactor the help command to also support this API.

JarrettSJohnson · 2025-05-13T16:24:11Z

@JarrettSJohnson I'd like to ask, should I proceed and refactor existing commands to adhere the declare_command API?

We can address that in a separate PR, if possible. Also I think some things were accidentally deleted, perhaps during the merging process? (e.g. testDeleteStates)

pslacerda · 2025-05-13T21:43:00Z

The tip of branch were based on an old commit. It took me several hours to finish this git chore task.

JarrettSJohnson

Generally looks fine to me, I was under the impression from the original PR that this would just be for extended, custom functions rather than native ones. I'm a little hesitant to change current commands just yet. I still need to play around with this first to ensure no functional changes.

JarrettSJohnson · 2025-05-13T22:41:42Z

testing/tests/api/helping.py

@@ -1,50 +0,0 @@
-import sys


Was this file intended to be removed here?

Yes. Is it ok?

Please keep for now since we still have code that uses this pattern.

modules/pymol/commanding.py

pslacerda · 2025-05-14T05:49:48Z

I still need to play around with this first to ensure no functional changes.

Me too. Except for the help command which I want to adjust soon, I'll take a time before any progress.

- Bio.pairwise2 has been deprecated -> Use Bio.Align.PairwiseAligner instead - SeqRecord constructor: Using a string as the sequence is deprecated -> Convert to Seq The changes are based on https://github.com/speleo3/pymol-psico/blob/ae2b92c262bc/psico/seqalign.py

pslacerda · 2025-07-15T20:14:21Z

If I remove the docs-on-comment feature will this be merged faster?

JarrettSJohnson · 2025-07-15T20:22:13Z

Apologies. Give me until the weekend, and I'll come back to this review. My involvement with incentive and open-source comes in waves, and I'm still pretty deep in 3.2/3.3 work.

pslacerda · 2025-07-21T20:31:55Z

I think this is important to get merged before 3.2 because otherwise will have a delay starting in the current incomplete feature awaiting 3.3.

modules/pymol/commanding.py

JarrettSJohnson · 2025-07-22T05:13:54Z

testing/tests/api/helping.py

@@ -1,50 +0,0 @@
-import sys


Please keep for now since we still have code that uses this pattern.

modules/pymol/commanding.py

JarrettSJohnson · 2025-07-22T05:33:30Z

modules/pymol/commanding.py

+                        break
+                    i += 1
+                else:
+                    i -= 3


I don't think I follow this line

Me neither, I just placed it here because was needed. I'd need to debug to remember exactly, but seems to me that it's related to rewind the cursor i when no doc is found.

JarrettSJohnson · 2025-07-22T05:34:56Z

modules/pymol/commanding.py

+
+        raise pymol.CmdException(f"Unsupported argument type {type}")
+
+    def parse_documentation(func):


I think this function probably needs its own unit test as well. The API to me is not inherently clear.

It is tested integrated with declare_command on test_declare_command_arg_docs.

I understand, but something like this probably deserves its own test too if possible. I'll make it optional, but I will likely add it later.

pslacerda · 2025-07-23T02:23:35Z

Give me some days and I'll fix according to your review.

As nobody is using this yet, including me, may cmd.declare_command be renamed to cmd.new or cmd.new_command?

JarrettSJohnson · 2025-07-23T06:58:16Z

Sure. new_command is fine. We already make use of cmd._new privately and it would be good for us not to be confused with the two.

pslacerda mentioned this pull request Apr 12, 2025

Renewed plugins #347

Open

pslacerda force-pushed the declare_command branch 2 times, most recently from 80eb0ce to 7c9f7ff Compare May 13, 2025 15:41

Refactor @cmd.declare_command

095e961

pslacerda force-pushed the declare_command branch from 7c9f7ff to 095e961 Compare May 13, 2025 21:23

JarrettSJohnson reviewed May 13, 2025

View reviewed changes

pslacerda1 and others added 9 commits May 31, 2025 21:18

Fix mixed string formatting

d0f8406

Fix quient default parameter

40d5053

Fix declare_command on Windows (schrodinger#459)

9fc0304

Fixed build on FreeBSD (schrodinger#461)

3140e28

Sorry for the messy commits

3aa351b

Merge branch 'master' into declare_command

a78deb4

Fix some bugs

3612302

Add support for Enum types in the declare_command

b2493c7

pslacerda mentioned this pull request Jun 23, 2025

Plugins needing refactor Pymol-Scripts/Pymol-script-repo#147

Open

JarrettSJohnson requested changes Jul 22, 2025

View reviewed changes

pslacerda1 added 2 commits July 22, 2025 11:35

Documents declare_command helpers

ab4c87e

Prefer f-strings

22ba8f8


		raise pymol.CmdException(f"Unsupported argument type {type}")

		def parse_documentation(func):

Improvements on the @cmd.declare_command API #448

Are you sure you want to change the base?

Improvements on the @cmd.declare_command API #448

Uh oh!

Conversation

pslacerda commented Apr 12, 2025

Uh oh!

JarrettSJohnson commented May 12, 2025

Uh oh!

pslacerda commented May 13, 2025

Uh oh!

pslacerda commented May 13, 2025

Uh oh!

pslacerda commented May 13, 2025

Uh oh!

JarrettSJohnson commented May 13, 2025

Uh oh!

pslacerda commented May 13, 2025

Uh oh!

JarrettSJohnson 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!

Uh oh!

pslacerda commented May 14, 2025

Uh oh!

pslacerda commented Jul 15, 2025

Uh oh!

JarrettSJohnson commented Jul 15, 2025

Uh oh!

pslacerda commented Jul 21, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

pslacerda commented Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JarrettSJohnson commented Jul 23, 2025

Uh oh!

Uh oh!

Improvements on the `@cmd.declare_command` API #448

Improvements on the `@cmd.declare_command` API #448

pslacerda commented Jul 23, 2025 •

edited

Loading