StudioInfinity/dyna3
2
0
Fork 2
Code Issues 86 Pull requests 1 Projects 2 Releases 4 Wiki Activity Actions

Polish behavior and code in the action system #155

Merged
glen merged 4 commits from Vectornaut/dyna3:actions-cleanup into main 2026-06-15 17:40:11 +00:00
Conversation 6 Commits 4 Files changed 3 +45 -23
Vectornaut commented 2026-06-11 08:15:47 +00:00
Member
Copy link

Make a few small improvements to the behavior and code of the action system.

Behavior

  • When Sphere and Point descriptors are given arguments, return interpretation errors instead of just ignoring the arguments.
  • Add the newlines that were missing from the loading script for the "Pointed" test assembly.
    • Now that Sphere and Point no longer accept arguments, interpretation of the miswritten script fails, as it always should have.
  • Correct a typo in the resolution error message for detailed descriptors.

Code

  • Remove the comment discussed here, which has now been incorporated into the wiki.
  • Take more advantage of the Self keyword in methods, and the self keyword in use declarations.
Make a few small improvements to the behavior and code of the action system. ### Behavior - When `Sphere` and `Point` descriptors are given arguments, return interpretation errors instead of just ignoring the arguments. - Add the newlines that were missing from the loading script for the "Pointed" test assembly. - Now that `Sphere` and `Point` no longer accept arguments, interpretation of the miswritten script fails, as it always should have. - Correct a typo in the resolution error message for detailed descriptors. ### Code - Remove the comment discussed [here](pulls/138#issuecomment-3801), which has now been incorporated into the [wiki](https://code.studioinfinity.org/StudioInfinity/dyna3/wiki/Selection#should-actions-consume-the-selection). - Take more advantage of the `Self` keyword in methods, and the `self` keyword in use declarations.
Remove the to-do comment about selection behavior
All checks were successful
/ test (pull_request) Successful in 3m49s
Details
c00c94c8a0 
This comment has been incorporated into the newly created wiki page
about selection, which discusses whether we actually want the behavior
the comment describes.
Vectornaut changed title from Polish behavior and code in the action system to WIP: Polish behavior and code in the action system 2026-06-11 22:46:58 +00:00
Vectornaut commented 2026-06-11 22:49:27 +00:00
Author
Member
Copy link

Converted to work in progress because I noticed a mistake in the error messages we return when a Sphere or Point descriptor gets arguments. The message refers to Sphere and Point as regulators, rather than elements.

Converted to work in progress because I noticed a mistake in the error messages we return when a `Sphere` or `Point` descriptor gets arguments. The message refers to `Sphere` and `Point` as regulators, rather than elements.
Add missing newlines to "Pointed" loading script
All checks were successful
/ test (pull_request) Successful in 3m50s
Details
3fc759763f 
Now that `Sphere` and `Point` no longer accept arguments, interpretation
of the miswritten script fails, as it always should have.
Vectornaut commented 2026-06-12 22:57:18 +00:00
Author
Member
Copy link

I decided to put off correcting the error messages until after the pull request that addresses issue #154, because that pull request is ready to go, and it moves the code that needs to be corrected. I've filed issue #156 as a reminder to correct the error messages.

I did update this pull request to fix an error that it reveals in one of the test assembly loading scripts (commit 3fc7597). That update is reflected under the "Behavior" heading in the pull request description. I filed issue #157 to recommend an automated test that would've alerted us when the loading script stopped working.

If you think it's okay to put off correcting the error messages, feel free to merge once you've reviewed the update!

I decided to put off correcting the error messages until after the pull request that addresses issue #154, because that pull request is ready to go, and it moves the code that needs to be corrected. I've filed issue #156 as a reminder to correct the error messages. I did update this pull request to fix an error that it reveals in one of the test assembly loading scripts (commit 3fc7597). That update is reflected under the "Behavior" heading in the pull request description. I filed issue #157 to recommend an automated test that would've alerted us when the loading script stopped working. If you think it's okay to put off correcting the error messages, feel free to merge once you've reviewed the update!
Vectornaut changed title from WIP: Polish behavior and code in the action system to Polish behavior and code in the action system 2026-06-12 22:57:27 +00:00
glen commented 2026-06-12 23:02:23 +00:00
Owner
Copy link

I'd really rather not get into a habit of cluttering our commit history for the sake of an unfiled on-deck PR. Since these are both reasonably compact PRs, please can you just correct #156 in this PR so that it never happens in the main commit line, and rebase your upcoming PR? If that is for some reason really onerous, please let me know. Thanks for your efforts on this.

I'd really rather not get into a habit of cluttering our commit history for the sake of an unfiled on-deck PR. Since these are both reasonably compact PRs, please can you just correct #156 in this PR so that it never happens in the main commit line, and rebase your upcoming PR? If that is for some reason really onerous, please let me know. Thanks for your efforts on this.
glen commented 2026-06-12 23:02:40 +00:00
Owner
Copy link

And good catch, by the way.

And good catch, by the way.
Correct argument-count error messages
All checks were successful
/ test (pull_request) Successful in 3m50s
Details
d64919c39f 
The messages we were using before incorrectly described `Sphere` and
`Point` as regulators.
Vectornaut commented 2026-06-15 03:50:20 +00:00
Author
Member
Copy link

Since these are both reasonably compact PRs, please can you just correct #156 in this PR so that it never happens in the main commit line, and rebase your upcoming PR?

Done in commit d64919c! Ready to review again.

> Since these are both reasonably compact PRs, please can you just correct #156 in this PR so that it never happens in the main commit line, and rebase your upcoming PR? Done in commit d64919c! Ready to review again.
glen commented 2026-06-15 17:37:47 +00:00
Owner
Copy link

OK, seems to work fine, and the changes are quite small, and I have filed #158 about a design issue that this PR surfaces, so merging.

OK, seems to work fine, and the changes are quite small, and I have filed #158 about a design issue that this PR surfaces, so merging.
glen merged commit a3c0c067b2 into main 2026-06-15 17:40:11 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug
Something is not working

  
ci
Issue concerning the continuous integration system

  
design
Issue that entails architecture/design decisions

  
duplicate
This issue or pull request already exists

  
engine
Issue concerning the engine

  
enhancement
New feature

  
maintenance
Refactors andother behavior-orthogonal tasks

  
prospective
This will become an issue once a particular feature is implemented or pull request is merged

  
question
More information is needed

  
regression
A bug that's resurfaced or a feature that's accidentally been removed

  
stub
We need to fill in the details of this issue

  
todo
Planned step in implementation

  
ui
Issue concerning the user interface

  
wontfix
This won't be fixed

No labels
bug
ci
design
duplicate
engine
enhancement
maintenance
prospective
question
regression
stub
todo
ui
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
StudioInfinity/dyna3!155
No description provided.