Conversation
|
Did you know that there is a way to maintain a connection with the engine where, at the start, all files are sent and then on saves, only changed files are sent? I have to look up what the configuration was. |
|
@Guzzardo is referring to the |
|
It could be EDIT: Yeah, this is the one. |
|
Yes you also need to be watching the templates via the script @Guzzardo mentioned. I think we need to cleanup the docs on |
|
That sounds ideal.. Thanks @Guzzardo will check it out |
evanj
left a comment
evanj
left a comment
There was a problem hiding this comment.
I don't know anything about this sublime plugin but I had some general comments about the code. This looks reasonable to me; I had a few minor comments.
The biggest one: I think you only need one of the return values from generate_pertinent_file_names_and_map, but I don't know how painful it will be to change it.
|
|
||
| def run(self, edit): | ||
| self.url = get_url(self.settings) + self.COMMAND_URL | ||
| use_pertinent_load = self.settings.get('pertinent_load', False) |
There was a problem hiding this comment.
I think this variable only gets used once; any reason to not just inline it in the if statement below? Or define it immediately before it gets used? It will be easier to understand what controls this if statement when reading it, if the two parts are close together.
| params['file_names'] = json.dumps(self.generate_file_list()) | ||
| use_cache = self.settings.get('use_cache', DEFAULT_USE_CACHE_SETTING) | ||
| if not use_cache: | ||
| params["templates"] = json.dumps(self.generate_file_map()) |
There was a problem hiding this comment.
Both branches assign file_names and templates to params; It might be cleaner to do something like:
if use_pertinent_load:
file_names = ...
templates = ...
else:
file_names = ...
params = ...
params['file_names'] = json.dumps(file_names)
params['templates'] = json.dumps(templates)
This would reduce duplication on the two branches
| self.partner = self.partner.replace("_templates", "") | ||
|
|
||
| def generate_pertinent_file_names_and_map(self, template_filename): | ||
| template_filename = template_filename.replace(self.path + '/', '') |
There was a problem hiding this comment.
Is this trying to get just the file name part of this? It seems a bit weird to me to use .replace, since that implies that "self.path" might appear somewhere at the beginning. Would os.path.basename do what you want?
|
|
||
| def generate_pertinent_file_names_and_map(self, template_filename): | ||
| template_filename = template_filename.replace(self.path + '/', '') | ||
| image_regex = "/img/" + self.partner + "/" + "(.*?)['\"]" |
There was a problem hiding this comment.
Combine the "/" with the rest into "/img/" + self.partner + "/(.*?)['\"]" ? slightly simpler
also to be anal-retentive: self.partner might include characters that are treated specially in regexes; I know it isn't supposed to, but if it does will this break in some catastrophic way? It might be fine.
You could use re.compile to check the pattern for errors here, and to compile it once, but for something like this: do whatever you think is simpler.
| all_images_required = [] | ||
| # go through all files this template requires | ||
| # we get all the filenames, their contents, and images | ||
| for file_name in all_file_names_required: |
There was a problem hiding this comment.
Can you get rid of this loop? It only ever contains template_filename? OH; you modify this list below. I see. That works? okay then never mind
| all_file_names_required.append(file_name) | ||
| contents = read_file(os.path.join(self.path, file_name)) | ||
| file_map[file_name] = contents | ||
| except: |
There was a problem hiding this comment.
again I would only catch the specific exception: this should "crash" if you have a typo or something.
| all_file_names_required += all_images_required | ||
|
|
||
| # lastly, we need to go through the other campaign files that accompany the html | ||
| template_base_name = template_filename.split('.')[0] |
There was a problem hiding this comment.
You might want to prefer os.path.splitext because it will "correctly" handle files named things like .profile. In this case it might not matter, so maybe never mind. https://docs.python.org/2/library/os.path.html#os.path.splitext
| template_base_name = template_filename.split('.')[0] | ||
| for postfix in ['.tracking', '.txt', '.yaml']: | ||
| if postfix == '.txt': | ||
| file_name = "_".join([self.action, "subject", self.subaction]) + postfix |
There was a problem hiding this comment.
This could just be self.action + "_subject_" + self.subaction + postfix which feels a bit simpler? No strong opinion though.
| if postfix == '.txt': | ||
| file_name = "_".join([self.action, "subject", self.subaction]) + postfix | ||
| else: | ||
| file_name = template_base_name+postfix |
| else: | ||
| file_name = template_base_name+postfix | ||
| try: | ||
| all_file_names_required.append(file_name) |
There was a problem hiding this comment.
if you get rid of all_file_names_required you can remove this line
5 participants
Right now, every time we want to render a template locally (this is the fastest way we can iterate on template), the sublime plugin reads in EVERY image and file from your local partner repo and sends it to the engine!
We do need to send a lot of information to the engine to preview, but not the whole repo. We should be able to parse through and know what files/images the preview would require, and just read/send those. That's what this does.
It's an intricate parser, so I have a flag
pertinent_loadthat's set default to false in case it doesn't work flawlessly (sorta will though). If you want to use it you can switch it on in the triggermail sublime plugin settings.This is helpful for rendering templates for partners with big repos. They often time out/take forever