Skip to content

Skip post-book actions if we can #673

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

Merged
merged 8 commits into from
Mar 18, 2019
Merged

Skip post-book actions if we can #673

merged 8 commits into from
Mar 18, 2019

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Mar 6, 2019

Skips some of the actions that we take after building all branches of a
book if we can get away with skipping them and reworks some of the
logging. This should marginally speed up the --all build and and make
it much easier to spot which books are rebuilt in which build by
suppressing a lot of the logging noise.

Skips some of the actions that we take after building all branches of a
book if we can get away with skipping them and reworks some of the
logging. This should marginally speed up the `--all` build and and make
it *much* easier to spot which books are rebuilt in which build by
suppressing a lot of the logging noise.
@nik9000 nik9000 requested review from a user and ddillinger March 6, 2019 17:24
@nik9000 nik9000 added the WIP label Mar 6, 2019
@nik9000
Copy link
Member Author

nik9000 commented Mar 6, 2019

I'm labelling this WIP because I'd like to add some integration testing that uses the infrastructure from #659.

@nik9000 nik9000 removed the WIP label Mar 11, 2019
@nik9000
Copy link
Member Author

nik9000 commented Mar 11, 2019

I've added some more tests around building multi-branched books. This is nice because multi-branched books are actually the most common type and we didn't have an integration tests for them.

@Jarpy, could you have this a look when you get a chance?

@nik9000
Copy link
Member Author

nik9000 commented Mar 12, 2019

I think I could build on this to do a few other neat things like only changing the parts of the sitemap for changed books.

ls $$(dirname $(2)); \
false; \
}
grep -v $(1) $(2) > /dev/null || { \
Copy link

Choose a reason for hiding this comment

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

grep -q saves you redirecting stdout and is a potentially an optimization because it short-circuits.

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'll do it!

define GREP_V=
# grep for a string in a file, outputting the whole file if there *is*
# a match.
[ -e $(2) ] || { \
Copy link

Choose a reason for hiding this comment

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

Is the false return the most important factor here, or the text output?

To put it another way, is this function assert_not_in_file or print_file_if_unmatched?

Copy link
Member Author

Choose a reason for hiding this comment

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

assert_not_in_file. But I need to print the file so I can figure out what is going on.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think assert_not_in_file is a better name for this? I'm so used to thinking about grep -v but the output on failure is different.

Copy link

Choose a reason for hiding this comment

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

I don't think it's a big deal, but after reading the function name and the comment, I still wasn't sure which part of the function was the core and which was the side-effect. It's not a big public API, so just tweaking the comment a bit would probably be fine, too.

I don't love the name GREP_V, because it reveals the implementation, but again, this a Makefile, not some hallowed, live-with-it-forever public API.

My question was mostly to satisfy my curiosity, rather than to demand changes.

@ghost ghost self-requested a review March 17, 2019 23:07
@nik9000 nik9000 merged commit 3fbfce1 into elastic:master Mar 18, 2019
@nik9000
Copy link
Member Author

nik9000 commented Mar 18, 2019

Thanks for reviewing @Jarpy!

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.

1 participant