Changes to second exercise in 09 issue #105 #106
Conversation
…ged dash in file name to underscore to match file names in code. Episode 09: changed mispelling of conforms.
…ationale for the creation of an archive and how it will be used. This will help students with this final exercise by giving them context. In addition this commit changes the Makefile solution for this exercise. Here are the changes, 1) copy files using rsync which moves books into a books directory in the archive folder, 2) rm the archive folder once the archive.tar.gz file is created. This avoids leaving two copies of your files in one directory which seems like it could lead to confusion.
There was a problem hiding this comment.
Thanks for the work! It's made me think hard about this section - I didn't write the lesson so I'm also considering what the best practice and learning objectives would be!
As a final thought (and this is intended to be constructive), you might find a read through this quite informative https://chris.beams.io/posts/git-commit
Thanks
_episodes/09-conclusion.md
Outdated
| > | ||
| > | ||
| > Edit the Makefile to create an archive file of your project. Add new rules, update existing | ||
| > rules, and add new macros to: |
There was a problem hiding this comment.
We haven't used the term "macro" yet. I think we should stick to using "variable" for consistency.
There was a problem hiding this comment.
Agreed, "macro" shows up in the PNG exercise too and I was confused by that.
There was a problem hiding this comment.
It's introduced in episode 6 as an equivalent term for variable used in some versions of Make. However, I've just updated episode 9 to use variable to avoid confusion.
_episodes/09-conclusion.md
Outdated
| > * Create an archive, `zipf_analysis.tar.gz`, of this directory. The | ||
| > bash command `tar` can be used, as follows: | ||
| > | ||
| > ~~~ | ||
| > $ tar -czf zipf_analysis.tar.gz zipf_analysis | ||
| > ~~~ | ||
| > {: .bash} | ||
| > | ||
| > | ||
| > * Remove the `zipf_analysis` directory once the `zipf_analysis.tar.gz` file has been created. |
There was a problem hiding this comment.
This would "break" the dependency checking such that make all would never be up to date, and would always run, whereas the idea is only to update targets when their dependencies have changed. I would leave this section as it was.
There was a problem hiding this comment.
Good point about the dependencies.
I would suggest leaving this zip function out of make all. When someone downloads the tar.gz archive, unzips and runs make they will have two identical copies, one in the base directory and one in "zipf_analysis" (and technically one in the tar.gz file itself). I think this is potentially confusing and at the very least it wastes space and time when directories are large.
I realize this is just a toy example so it's not that important.
There was a problem hiding this comment.
I would leave the zip function in make all.
Continuing the narrative beyond the scope of the lesson - after we have done our analysis and archived it (this is the end of the lesson/exercise), should someone download the tar.gz file, unpack it, and run make all, the targets will be up-to-date so there will be 'nothing to be done'.
_episodes/09-conclusion.md
Outdated
| > * Update `all` to create `zipf_analysis.tar.gz`. | ||
| > * Remove `zipf_analysis` and `zipf_analysis.tar.gz` when `make | ||
| > clean` is called. | ||
| > * Remove `zipf_analysis.tar.gz` when `make clean` is called. |
There was a problem hiding this comment.
I would leave the original in light of the previous comment
_episodes/09-conclusion.md
Outdated
| > * Copy all our code, data, plots and the Zipf summary table to this | ||
| > directory. | ||
| > directory. The rsync command can be used to copy files and their |
There was a problem hiding this comment.
I would be keen to avoid introducing new commands unless strictly necessary. We could use a new variable in the Makefile to refer to the books directory, and just use the cp -r command. We then might as well use it in the patsubst rules too, so the start of the Makefile would look like this:
include config.mk
TXT_DIR=books
TXT_FILES=$(wildcard $(TXT_DIR)/*.txt)
DAT_FILES=$(patsubst $(TXT_DIR)/%.txt, %.dat, $(TXT_FILES))
PNG_FILES=$(patsubst $(TXT_DIR)/%.txt, %.png, $(TXT_FILES))
RESULTS_FILE=results.txt
ZIPF_DIR=zipf_analysis
ZIPF_ARCHIVE=$(ZIPF_DIR).tar.gz
## all : Generate archive of code, data, plots and Zipf summary table.
.PHONY : all
all : $(ZIPF_ARCHIVE)
$(ZIPF_ARCHIVE) : $(ZIPF_DIR)
tar -czf $@ $<
$(ZIPF_DIR): Makefile config.mk $(RESULTS_FILE) \
$(DAT_FILES) $(PNG_FILES) $(TXT_DIR)\
$(COUNT_SRC) $(PLOT_SRC) $(ZIPF_SRC)
mkdir -p $@
cp -r $^ $@
touch $@
It would also copy the licence file so fixes another concern.
There was a problem hiding this comment.
Nice solution. It requires changing the makefile in the previous episodes so that books is a wildcard.
There was a problem hiding this comment.
Also we're going to need to change the dependencies diagrams.
There was a problem hiding this comment.
I'm not sure if I missed something? I wouldn't bother changing the Makefiles from previous episodes. We've only introduced the TXT_DIR variable to use in the recently modified ZIPF_DIR rule. So it makes sense to use the variable to replace text in e.g. in TXT_FILES given it now exists, but I don't see why previous Makefiles would need changing?
| @@ -13,12 +13,13 @@ all : $(ZIPF_ARCHIVE) | |||
|
|
|||
| $(ZIPF_ARCHIVE) : $(ZIPF_DIR) | |||
| tar -czf $@ $< | |||
| rm -rf $(ZIPF_DIR) | |||
There was a problem hiding this comment.
See above suggestion for revised Makefile.
Add text to motivate challenge 2 in episode 09. This helps explain the purpose of making an archive file. Add text to give more detailed instructions on how to solve the challenge. There are a lot of steps in this challenge! Fix the `books` directory not being copied into the archive file correctly in the solution. Issue: swcarpentry#105 Pull request review: swcarpentry#106
Minor edits to improve wording and fix typo. Issue: swcarpentry#105
|
Thanks and sorry it took so long to merge - I didn't realise this was still |
Add text to motivate challenge 2 in episode 09. This helps explain the purpose of making an archive file. Add text to give more detailed instructions on how to solve the challenge. There are a lot of steps in this challenge! Fix the `books` directory not being copied into the archive file correctly in the solution. Issue: #105 Pull request review: #106
Better motivate archive creation in final exercise. Fix #105.
Changing the text of the archiving exercise in the conclusion chapter 09 and changing the solution makefile for that exercise. The changes address issue #105.
There is still a problem in my solution: the books directory in the archive does not include the license file that goes with the books txt files.