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

Smarter check #1759

Merged
merged 9 commits into from May 31, 2015
Merged

Smarter check #1759

merged 9 commits into from May 31, 2015

Conversation

@ralsina
Copy link
Member

@ralsina ralsina commented May 27, 2015

Fix #1758 and hopefully breaks nothing ;-)

ralsina added 2 commits May 27, 2015
@ralsina
Copy link
Member Author

@ralsina ralsina commented May 27, 2015

@felixfontein I'd like your review of this one

ralsina added 2 commits May 27, 2015
def _call_nikola_list(site):
files = []
for task in generate_tasks('render_site', site.gen_tasks('render_site', "Task", '')):
files.extend(task.targets)

This comment has been minimized.

@felixfontein

felixfontein May 27, 2015
Contributor

This won't work if any of the generated tasks has a delayed task loader, but a) probably no plugin author does that, and b) I have to go through that anyway when adjusting this for the earlytask_impl branch, so I vote for leaving this as is for the moment.

This comment has been minimized.

@ralsina

ralsina May 27, 2015
Author Member

Ack.



def real_scan_files(l, site):
def real_scan_files(site):

This comment has been minimized.

@felixfontein

felixfontein May 27, 2015
Contributor

This function is also called from orphans.py and github_deploy.py.

This comment has been minimized.

@ralsina

ralsina May 27, 2015
Author Member

Done

fname = task.split(':', 1)[-1]
for fname in _call_nikola_list(site):
fname = fname.strip()
if output_folder in fname:

This comment has been minimized.

@felixfontein

felixfontein May 27, 2015
Contributor

startwith() is better than in in this context, as maybe some file cache/bla/output/bla is generated, which also contains output.

This comment has been minimized.

@ralsina

ralsina May 27, 2015
Author Member

Done

@@ -262,7 +252,7 @@ def analyze(self, task, find_sources=False, check_remote=False):
self.logger.warn("Broken link in {0}: {1}".format(filename, target))
if find_sources:
self.logger.warn("Possible sources:")
self.logger.warn("\n".join(_call_nikola_list(self.l, self.site, ["--deps", task])))
self.logger.warn("\n".join(_call_nikola_list(self.site)))

This comment has been minimized.

@felixfontein

felixfontein May 27, 2015
Contributor

The output of nikola list --deps <taskname> is quite different than the one from nikola list --all, so this is not a valid substitution. Actually, I think we need some support from doit here, if we don't want to repeat some of its code or use internal APIs. @schettino72: what do you think (or does that already exist?) about having a simple way in doit to get essentially the output of doit list --all --deps (including a list of targets per task) in a nice way?

This comment has been minimized.

@ralsina

ralsina May 27, 2015
Author Member

Good catch. Same thing as before, I just need to check task.file_dep, no big deal. Will fix it tomorrow.

This comment has been minimized.

@ralsina

ralsina May 27, 2015
Author Member

Done.

This comment has been minimized.

@schettino72

schettino72 May 27, 2015
Member

As I said before, the doit list supports a template param to format its output (it seems I didnt document this parameter - sorry). Currently the template can only output values for: name, status and doc. I guess the best option would be to extend supported values that can appear in the template. Or maybe support a json output?

There is also the doit info command that outputs the targets and dependencies but lacks a formatting and a --all options...

This comment has been minimized.

@felixfontein

felixfontein May 28, 2015
Contributor

I'd prefer JSON output; that's easy to parse and one can also easily add more info without breaking existing clients.

# Maybe we should just examine all HTML files
output_folder = self.site.config['OUTPUT_FOLDER']
for fname in _call_nikola_list(self.site):
if output_folder in fname and '.html' in fname:

This comment has been minimized.

@felixfontein

felixfontein May 27, 2015
Contributor

What about using endswith() here instead of (the second) in? Just in case someone likes to use .html in directory names :)

This comment has been minimized.

@ralsina

ralsina May 27, 2015
Author Member

well [-5:] but same idea :-)

def _call_nikola_list(site):
files = []
deps = defaultdict(list)
for task in generate_tasks('render_site', site.gen_tasks('render_site', "Task", '')):

This comment has been minimized.

@Kwpolska

Kwpolska May 28, 2015
Member

Is this safe? Are we not doing it twice?

We should probably talk to our task loader anyways.

This comment has been minimized.

@ralsina

ralsina May 28, 2015
Author Member

It works anyway. To talk to our taskloader we'd have to move it out of __main__ first.

This comment has been minimized.

@Kwpolska

Kwpolska May 28, 2015
Member

You can just talk to self.site.doit.task_loader. You also can save the generated tasks there, if any.

This comment has been minimized.

@ralsina

ralsina May 28, 2015
Author Member

I tried it, see no difference, and I don't understand internals well enough, maybe.

So, proposing to merge this as-is.

@felixfontein
Copy link
Contributor

@felixfontein felixfontein commented May 28, 2015

Before merging: what is that problem with the nikola check -l failure in the tests running on AppVeyor? Is it something Windows specific?

for target in task.targets:
deps[target].extend(task.file_dep)
for task in generate_tasks('post_render', site.gen_tasks('render_site', "LateTask", '')):
files.extend(task.targets)

This comment has been minimized.

@felixfontein

felixfontein May 28, 2015
Contributor

Don't you need the lines updating deps[target] here as well?

This comment has been minimized.

@ralsina

ralsina May 28, 2015
Author Member

You are correct. Good catch! Fixing...

@ralsina
Copy link
Member Author

@ralsina ralsina commented May 28, 2015

The output of the demo site changes because I fixed a broken link generated by the listings plugin, I think.

@felixfontein
Copy link
Contributor

@felixfontein felixfontein commented May 28, 2015

Ah ok. Then everything's fine from my POV :)

ralsina added a commit that referenced this pull request May 31, 2015
@ralsina ralsina merged commit 423f3a7 into master May 31, 2015
0 of 3 checks passed
0 of 3 checks passed
continuous-integration/appveyor AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
@ralsina ralsina deleted the smarter-check branch May 31, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants