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

Updating Streamline branch #89

Open
wants to merge 10 commits into
base: streamline
Choose a base branch
from

Conversation

Dreamersoul
Copy link
Member

No description provided.

@Maryom
Copy link
Contributor

Maryom commented Sep 29, 2016

Salaam,

Great job :) Just a small note, in app_template.swift I think it's important to add author, current date, and current year :

//
//  {{ app.name }}.swift
//  Appz
//
//  Created by {{ author.name }} on {{currentDate}}.
//  Copyright © {{currentYear}} kitz. All rights reserved.
//

Does anyone agree?

@Mazyod
Copy link
Member

Mazyod commented Sep 29, 2016

The files don't really have an author, so it's better be kept as is. Sure, the current_year would be a nice thing.

Copy link
Member

@Mazyod Mazyod left a 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)
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

@Maryom
Copy link
Contributor

Maryom commented Sep 29, 2016

If you don't mind I have a question what about using Swift script as mentioned here

And as mentioned:
"Even if this isn’t the exact solution for the exact problem that you’re trying to solve, the concepts that we will go over here will be applicable to other Swift scripts that you might want to write."

I think it will be easier in testing.

@Mazyod
Copy link
Member

Mazyod commented Sep 29, 2016

@Maryom What is your question?

@Mazyod
Copy link
Member

Mazyod commented Sep 29, 2016

@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
Copy link
Contributor

Maryom commented Sep 29, 2016

@Mazyod You are right but I liked their way that why I shared it here. Do you think their way will be easier in testing? I think so ..

@Mazyod
Copy link
Member

Mazyod commented Sep 29, 2016

@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 ,
Copy link
Member Author

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'

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.

None yet

3 participants