-
Notifications
You must be signed in to change notification settings - Fork 78
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
Updating Streamline branch #89
base: streamline
Are you sure you want to change the base?
Conversation
Salaam, Great job :) Just a small note, in app_template.swift I think it's important to add author, current date, and current year :
Does anyone agree? |
The files don't really have an author, so it's better be kept as is. Sure, the |
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.
Everything looks great! I just have one small observation in the actions method, especially since it's quite long.
return definition | ||
|
||
def create_action(self): | ||
action = "case .{}(".format(self.name) |
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.
I am not exactly sure why you aren't using templates here? I think it's much cleaner if you create smaller template files, like action_template.txt or something.
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.
I am planning on refactoring this code since it's really looking awful, your suggestion is way better than what I had in mind, gonna work on it next time I can.
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.
the actions are now being generated using a jinja template, should be way better now.
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.
@Dreamersoul Definitely starting to look a lot more organized.
I also imagined that every part would be a template, and then we can have a template loader globally, TemplateLoader.get("blah")
. Something like parameter_enum_template.txt
, parameter_arg_template.txt
, action_enum_template.txt
, action_definition_template.txt
, ... etc. This will make sure the script doesn't deal with any string manipulation, and trust Jinja for all that stuff.
This might be just a burden, so I leave it up to you whether it is worth it or not.
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.
I love how this is going, I'll try to make it work after writing tests.
If you don't mind I have a question what about using Swift script as mentioned here And as mentioned: I think it will be easier in testing. |
@Maryom What is your question? |
@Maryom It is up to the contributor to decide which language he prefers. It almost makes no difference what language is used for the script, as long as it provides the functionality we require and is widely available on most machines. |
@Maryom Why do you think it's easier to test? I'm sure it's harder. Testing in python is so easy, you literally do it like this using py.test: class AppzTests:
def test_1_plus_1():
assert 1 + 1 == 2 |
for parameter in self.parameters: | ||
parameter_string += "let {},".format(parameter.name) | ||
if len(self.parameters) > 0: | ||
parameter_string = parameter_string[:-1] # remove tailing , |
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.
consider using the join method like here instead of appending and removing the trailing ","
", ".join(["hello", "world", "last string"])
# 'hello, world, last string'
No description provided.