Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for subcommands in the API #254

Closed
wants to merge 3 commits into from
Closed

Conversation

stveit
Copy link
Contributor

@stveit stveit commented Jun 13, 2024

Fixes #255

Suggestion for supporting subcommands. Adds a constant tuple where commands that support subcommands can be added (for us this will contain pm). If a command is registered in that tuple, the dispatcher will attempt to add the first argument to the command and look for functions matching this. If main command is pm and subcommand is details then it will look for do_pm_details

I imagine the Zino1ServerProtocol subclass would register pm as a subcommand by overriding the tuple

This is preliminary work for implementing the planned maintenance API commands, so i dont think this needs to be part of the changelog. The commands themselves will be, however

@stveit stveit self-assigned this Jun 13, 2024
Copy link

github-actions bot commented Jun 13, 2024

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ PYTHON black 2 0 0.71s
✅ PYTHON isort 2 0 0.23s
✅ PYTHON ruff 2 0 0.02s

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

Copy link

github-actions bot commented Jun 13, 2024

Test results

    3 files      3 suites   51s ⏱️
  386 tests   386 ✅ 0 💤 0 ❌
1 158 runs  1 156 ✅ 2 💤 0 ❌

Results for commit 7fbd015.

♻️ This comment has been updated with latest results.

Copy link

codecov bot commented Jun 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.04%. Comparing base (6c30fd2) to head (7fbd015).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #254      +/-   ##
==========================================
+ Coverage   98.99%   99.04%   +0.05%     
==========================================
  Files          50       50              
  Lines        6739     6747       +8     
==========================================
+ Hits         6671     6682      +11     
+ Misses         68       65       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stveit stveit marked this pull request as draft June 13, 2024 08:01
Copy link
Contributor

@hmpf hmpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine start, now you "just" need to update the tests :)

@hmpf hmpf self-requested a review June 13, 2024 11:56
@stveit stveit marked this pull request as ready for review June 13, 2024 11:58
@lunkwill42 lunkwill42 self-requested a review June 13, 2024 12:30
@johannaengland johannaengland self-requested a review June 14, 2024 09:21
Copy link

sonarcloud bot commented Jun 18, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it's a fine attempt, I feel like this just complicates future maintenance.

Specifically, I'm not comfortable with having to explicitly add new methods to a an extra config tuple, located potentially far away from the implementation itself, to mark them as special. Marking these functions using a decorator would be nicer.

However, at this point, I might suggest two different approaches:

  1. Add more magic to the dispatcher code so it finds valid commands from method names, automatically understanding do_x_y() as an implementation of the command X Y while do_x() is a different command.

  2. Or, toss the magic stuff I wrote earlier and switch to a scheme involving explicit routing of commands using decorators. Think similarly to flask routes, e.g.

    @route("PM")
    def do_pm(self):
        ...
    
    @route("PM ADD")
    def do_pm_add(self, arg1, arg2):
        ...

I would pair on something like this, if desirable :)

@stveit
Copy link
Contributor Author

stveit commented Jul 1, 2024

#247 is used instead

@stveit stveit closed this Jul 1, 2024
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.

Rework the Zino1BaseServerProtocol command dispatcher to facilitate commands with subcommands
4 participants