Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 64 additions & 11 deletions triggermail_templates.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ def get_extra_params(self):

def run(self, edit):
self.url = get_url(self.settings) + self.COMMAND_URL
use_pertinent_load = self.settings.get('pertinent_load', False)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


template_filename = self.view.file_name()
if not template_filename:
Expand All @@ -66,13 +67,6 @@ def run(self, edit):

self.dissect_filename(template_filename)

# get file names
file_names = json.dumps(self.generate_file_list())
use_cache = self.settings.get('use_cache', DEFAULT_USE_CACHE_SETTING)

print("Attempting to render %s for %s" % (self.action, self.partner))
print("url is %s" % self.url)

params = dict(product_count=self.settings.get("product_count", 3),
partner=self.partner,
action=self.action,
Expand All @@ -85,11 +79,23 @@ def run(self, edit):
use_dev='dev.' in template_filename,
generation=getattr(self, 'generation', 0),
variant_id=getattr(self, 'variant_id', ''),
subaction=getattr(self, 'subaction', ''),
file_names=file_names)
subaction=getattr(self, 'subaction', ''))

print(params)
if not use_cache:
params["templates"] = json.dumps(self.generate_file_map())

if use_pertinent_load:
file_names, file_map = self.generate_pertinent_file_names_and_map(template_filename)
params['file_names'] = json.dumps(file_names)
params['templates'] = json.dumps(file_map)
else:
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())
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


print("Attempting to render %s for %s" % (self.action, self.partner))
print("url is %s" % self.url)

try:
nqe = self.settings.get("nqe")
assert nqe
Expand Down Expand Up @@ -128,6 +134,53 @@ def dissect_filename(self, template_filename):
self.partner = self.settings.get("partner", self.partner) or self.partner
self.partner = self.partner.replace("_templates", "")

def generate_pertinent_file_names_and_map(self, template_filename):
template_filename = template_filename.replace(self.path + '/', '')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

image_regex = "/img/" + self.partner + "/" + "(.*?)['\"]"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

file_map = dict()
all_file_names_required = [template_filename]
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:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# some templates include html files that don't exist. Just because they don't exist, doesn't
# mean it should automatically fail the command, they are most likely in jinja "if" statements
try:
contents = read_file(os.path.join(self.path, file_name))
except:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you catch the specific exception, instead of all exceptions? This catch all will hide bugs. Additionally I'm not sure if it is useful for a sublime plugin, but logging the exception with a message that you are ignoring it is frequently useful when debugging

continue
file_map[file_name] = contents
# this regex finds all the other files the html file depends on. Because we're thenadding to
# the loop we're cycling through, this can be recursive
files_required_by_file = [file_name for _,file_name in \
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anal retentive suggestion of _, file_name with an extra space

re.findall("{%\s*(include|extends|import)\s*['\"](.*?)['\"]", contents)]
all_file_names_required += [x for x in files_required_by_file if x not in all_file_names_required]
images_required_by_file = re.findall(image_regex, contents)
all_images_required += [img for img in re.findall(image_regex, contents) if img not in all_images_required]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would theoretically be better for all_images_required to be a set(), since testing if something is in a set is constant time. In fact, this could then become:

all_images_require.update(re.findall(image_regex, contents)) (I think)


for img in all_images_required:
img_path = os.path.abspath(os.path.join(self.image_path, img))
contents = encode_image(img_path)
file_map[img] = contents

all_file_names_required += all_images_required
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to even do this? I think file_map.keys() already effectively has this? I would remove this.


# lastly, we need to go through the other campaign files that accompany the html
template_base_name = template_filename.split('.')[0]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for postfix in ['.tracking', '.txt', '.yaml']:
if postfix == '.txt':
file_name = "_".join([self.action, "subject", self.subaction]) + postfix
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could just be self.action + "_subject_" + self.subaction + postfix which feels a bit simpler? No strong opinion though.

else:
file_name = template_base_name+postfix
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spaces around +

try:
all_file_names_required.append(file_name)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you get rid of all_file_names_required you can remove this line

contents = read_file(os.path.join(self.path, file_name))
file_map[file_name] = contents
except:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again I would only catch the specific exception: this should "crash" if you have a typo or something.

sublime.error_message("trouble loading file " + file_name)

return (all_file_names_required, file_map)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can just return file_map, right? (see above re: all_file_names_required)


def generate_file_map(self):
# Read all the files in the given folder.
# We gather them all and then send them up to GAE.
Expand Down