-
Notifications
You must be signed in to change notification settings - Fork 6
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
Move Package Management Logic to a Module #114
base: master
Are you sure you want to change the base?
Conversation
With the eventual intent on moving some `main.py` functionality into modules, it will be useful to reuse some of core's logic for temporary cli use. Thus, we will need to spin up temporary instances of `Halibot` to load config, load modules, etc. This adds an optional keyword `config` to `Halibot`, which sets a config dict as supplied. By default, this value is empty, as it was previously. This also adds an optional keyword `configfile` to `Halibot`, to specify an alternate configuration file to load on `._load_config()`. Therefore, this method has also been updated to accept an optional parameter `configfile` parameter of its own, in the event some external code wants to load an alternate code without running the full start process. The default config file remains `config.json`. In theory, no changes to other code is necessary. **NOTE:** With this change, `config` is no longer a static class variable, and is now purely an instance variable. This probably shouldn't affect anything, but multiple `Halibot` objects will no longer have the same config dict object.
aab86aa
to
513872a
Compare
Pushed a couple of small fixes:
|
self.config = json.loads(f.read()) | ||
halibot.packages.__path__ = self.config.get("package-path", []) | ||
|
||
def _write_config(self): | ||
with open(os.path.join(self.workdir, "config.json"), "w") as f: | ||
def _write_config(self, configfile="config.json"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we pull out the "config.json"
string and make it a constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"of": "core:PackageManager" | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about having the CLI work as a halibot instance, where there is one agent that receives the commands from the CLI and and then the various default
|
That was sort of what I was trying to work towards with this PR. The obvious exception would be a I think all we would have to do is have the dummy cli-agent " I need to refresh myself a bit first on this PR to be honest.
I went back and forth on that one debating if it should be a different file like
That's the goal. I'd like little-to-no logic in |
THIS IS A VERY RFC PATCH, DO NOT MERGE
Moral of this PR: Why do things in
main.py
, when we can share that logic with a module?This is a crazy hacky RFC to demonstrate a method of implementing logic that is meant to be shared between the CLI and an actual running instance. Due to halibot-extra/irc#3 , I'm not able to test the runtime portion yet, but surprisingly the CLI portion does.
General idea:
Add a new
-instances
list toconfig.json
calledcli-instances
, which are only ever loaded frommain.py
. Only modules that implement.cli()
(final name TBD) and.cli_receive()
(also needs fixing) should be ever listed there. The.cli()
call takes in the command subparser frommain.py
, and adds whatever commands it supports there. The.cli_receive()
makes the actual call.Why.
Well, doing it this way meant I only needed to change how to print out information. Plus, we don't need to depend on routing or any other weird stuff yet to figure out when the CLI commands need to show up at all. For now, this will lead to a much simpler and smaller
main.py
, and the module side of things isn't any more complex than it was before.Look forward to a
!config
module using the similar pattern coming soon, probably based off of this branch.Other notes:
for-0.3.0
branch, or bump version shortly after