Spiked a core-engine with example predicate and action#8
Spiked a core-engine with example predicate and action#8
Conversation
|
I have no idea how to add an extra person to a PR. @scottgreenup |
|
|
||
| # Skip comments | ||
| if line.startswith('#'): | ||
| if line.startswith('#') or not len(line) > 1: |
There was a problem hiding this comment.
Maybe that condition could be rewritten?
len(line) == 0 or len(line) <= 0
There was a problem hiding this comment.
Can I do just 'len(line)' is python? Like with truthy falsy values?
There was a problem hiding this comment.
Yeah, not len(line) works too. Not sure which is better.
There was a problem hiding this comment.
You can also do not line because "" is considered falsey
| def main(): | ||
| config_files = [ | ||
| '~/.filedrc' | ||
| path.join(path.dirname(__file__), 'resource/filedrc') |
There was a problem hiding this comment.
So we'll be getting a user's config from the "resource/filedrc" file? Good for testing but maybe we need to allow a certain file/folder for all user's configs.
Okay for now, just something that would need to be discussed.
There was a problem hiding this comment.
Oh. I should have mentioned that was just to wire up the example configure file.
We still need to figure out how we want to lay out the confit file.
ejwmoreau
left a comment
ejwmoreau
left a comment
There was a problem hiding this comment.
I think the src folder needs a init.py file in order to properly import the predicates/predicate.py file.
Maybe also init.py files in predicates and actions folders.
|
It's interesting you say that. PyCharm was complaining about not finding the file but python was executing this just fine. I might be missing the init.py file |
|
Huh, interesting |
| parts = line.split(':') | ||
| if len(parts) != 3: | ||
| logging.warning("{}:{}: Error reading line".format(filename, num)) | ||
| logging.warning("{}:{}: Error reading line".format(line, num)) |
There was a problem hiding this comment.
filename gives you the config file that is having a problem. The line itself should be part of the error, IMO.
scottgreenup
left a comment
scottgreenup
left a comment
There was a problem hiding this comment.
I like the general layout, but we need to discuss the layout/configuration from the user's perspective before you commit to this.
|
Should probably close this or merge it? |
|
@scottgreenup Did you hear Roger and I might work on this in the upcoming weeks? :P This PR is probably a good start, so I'm up for merging 🙂 |
3 participants
Work in progress.
This is how I imagine things would approximately look.
Let me know what you think.