everything #1

davidchambers · 2017-08-12T23:11:23Z

We've spent a lot of time developing the infrastructure surrounding the various Sanctuary projects. Here are some of the pull requests:

add documentation and Transcribe-powered prepublish script sanctuary#36 add documentation and Transcribe-powered prepublish script
license: update copyright information and automate future updates sanctuary#213 license: update copyright information and automate future updates
documentation: use version-specific project URLs sanctuary#324 documentation: use version-specific project URLs
lint: use a Markdown parser to check link definitions sanctuary#363 lint: use a Markdown parser to check link definitions
lint: ensure merge commit summary will not exceed 72 characters sanctuary#420 lint: ensure merge commit summary will not exceed 72 characters

When creating a new Sanctuary project we usually copy an existing makefile, a couple of shell scripts, and a dozen or so dev dependencies. There are two problems with this approach:

improving a shell script involves making the same change in multiple repositories; and
the same dozen or so dev dependencies must be kept up to date in multiple repositories.

To address these problems, this pull request brings together (and improves) a number of scripts currently found in various Sanctuary repositories.

These compare views provide an idea of the amount of common code which we'll be able to remove from the various Sanctuary repositories:

Note that the diff is less impressive for sanctuary-def, but this is because some of the infrastructure added to the Sanctuary repository has not yet propagated to other repositories.

Avaq · 2017-08-14T08:01:18Z

Makefile

+SHELL = /bin/bash
+
+NPM = npm
+SHELLCHECK = shellcheck


ShellCheck looks like a nice tool :)

written in Haskell, if you're into that sort of thing

I'm sold!

Avaq

I like this project a lot! I enjoy seeing advanced* bash scripting.

_{* By advanced, I mean anything I couldn't pull off myself.}

Avaq · 2017-08-14T08:02:05Z

README.md

@@ -0,0 +1,3 @@
+# sanctuary-scripts
+
+Shell scripts used in multiple Sanctuary projects.


Should there be more information here?

Quite possibly. What sort of information do you have in mind?

Avaq · 2017-08-14T08:05:35Z

bin/generate-readme

+set -euf -o pipefail
+
+transcribe() {
+  local -r project="$(node --print 'require("./package.json").repository.url.replace(/[.]git$/, "").split("/").slice(-2).join("/")')"


The repository field in a package.json doesn't have to be an object with a url property. It can be a string representation with a variety of formats. Do we want to support these formats? If not, we should mention that in the README.

I didn't know about the --print option in Node. That's great!

My goal is to make using these scripts in Sanctuary projects (and other projects with the same conventions) as convenient as possible. This means I'm willing to make the scripts less flexible in order to avoid configuration (in some cases default values can be used to let us have our 🍰 and eat it too).

We could add a lint-package-json script which performs some package.json-specific checks in addition to the sanity check performed by lint-json. It could assert that repository.url is present and that its value is suitable. What do you think of this idea?

Avaq · 2017-08-14T08:09:29Z

bin/generate-readme

+transcribe() {
+  local -r project="$(node --print 'require("./package.json").repository.url.replace(/[.]git$/, "").split("/").slice(-2).join("/")')"
+  local -r version="${VERSION?Environment variable not set}"
+  local -r url="https://github.com/$project/blob/v$version/{filename}#L{line}"


I see we rely explicitly on version being prefixed by v. I don't see a script for XYZ specified in this repo, so this means that one has to configure XYZ properly for sanctuary-scripts to work. Maybe we can add a release.sh script or something to this repo which ensures the correct tagging convention? Or if not, we should add a note to the README.

Good point. I had not noticed this unstated assumption. I had been unsure as to whether sanctuary-scripts should manage the xyz dependency; you've made it clear to me that it should.

Avaq · 2017-08-14T08:17:21Z

bin/lint-commit-message

+    echo 0 ;
+  } \
+  | head -n 1 \
+  | xargs expr 1 + 25 +


Rather than adding an arbitrary buffer to the PR number, can't we just add a single character buffer? So an additional reserved character to allow for the expansion of the PR number? That would mean that:

After the 1st PR, we reserve 9 slots
Once we reach 10 PR's, we reserve 990 slots
Once we reach 100 PR's, we reserve 9900 slots
...etc...

To me this approach seems less arbitrary, and more scalable. On the other hand, it might be overkill, and it may be upsetting to have 90% of all merge commit messages needlessly limited to -1 characters.

To me this approach seems less arbitrary, and more scalable.

I dislike arbitrariness. I like your suggestion. I will make the change. :)

Avaq · 2017-08-14T08:19:18Z

bin/lint-commit-message

+
+project_name() {
+  git config --get remote.origin.url \
+  | sed -e 's,^git@github[.]com:,,' -e 's,^https://github[.]com/,,' \


That's a nice way to avoid escape-hell (\\\\.) in this case. I will remember. :D

Also, using s,,, rather than s/// avoids lots of confusion when matching URLs. :)

Avaq · 2017-08-14T08:24:53Z

bin/update-copyright-year

+  exit 1
+fi
+
+mv "$updated" "$license"


Shouldn't this be forced? Won't bash complain that the file is already at this destination?

That's certainly a reasonable expectation. By default, though, mv will quite happily replace an existing file (there are flags for changing this behaviour).

Avaq · 2017-08-20T06:35:38Z

README.md

+
+## `lint-package-json`
+
+Asserts that __package.json__ exists and contains certain fields with


~~certain~~ some important? (to match check-required-files and remove air of mystery)

Avaq · 2017-08-20T06:40:47Z

README.md

+
+## `lint-json`
+
+Asserts that the specified JSON files exist and are neatly formatted.


It looks like the script doesn't do what this says. It should say something like: Asserts that the specified JSON files exist and have no syntax error, and then formats these files.

It really does assert that the files are neatly formatted. After reformatting a file it runs git --no-pager diff --exit-code -- "$file". The --exit-code flag is important here:

--exit-code Make the program exit with codes similar to diff(1). That is, it exits with 1 if there were differences and 0 means no differences.

Ahh. I see. But it does leave the working tree "dirty" (with neat files) afterwards?

Having put this script through its paces I no longer think involving git is a good idea. I'll rework the script to use cp and diff instead. This will avoid making a mess, and will allow linting of untracked JSON files.

Avaq · 2017-08-21T09:29:41Z

bin/release

+node_modules/.bin/xyz \
+  --increment "${increment}" \
+  --repo "git@github.com:${owner}/${name}.git" \
+  --script scripts/prepublish \


Now that there's way to configure some of these scripts, should this field remain hard-coded? Maybe

prepublish-script = scripts/prepublish

Allowing it to be emptied, similarly to the version-prefix option?

Good idea. I'll make this change. We can keep scripts/prepublish as the default value, and have check-required-files assert that the (explicitly or implicitly) specified file is present and executable.

I decided not to make this change after all, as we're already relying on convention for overriding the behaviour of scripts. To override sanctuary-lint, for example, one would create an executable at scripts/lint.

Note that this pull request includes a default prepublish script suitable for use in most Sanctuary projects.

Avaq

I noticed you squashed, so I gave it a last good look.

Avaq · 2017-10-29T08:47:08Z

README.md

+    ```json
+    {
+      "root": true,
+      "extends": ["./node_modules/sanctuary-style/eslint-es3.json"]


I think you can use a short notation, like "sanctuary-style/eslint-es3".

Thanks. I didn't know that. I'll keep it as it is for consistency with the handful of projects I updated over the weekend.

Avaq · 2017-10-29T09:21:06Z

README.md

+| `heading-level`               | `4`           | The `<h[1-6]>` level of headings transcribed from `heading-prefix` comments.  |
+| `heading-prefix`              | `#`           | The character which follows `//` to signify a heading to transcribe.          |
+| `comment-prefix`              | `.`           | The character which follows `//` to signify documentation to transcribe.      |
+| `version-tag-prefix`          | `v`           | The prefix of annotated version tags (use _null_ for `X.Y.Z` format).         |


It's not immediately clear to me what is meant by use null. Do I set the value to literal null? Or do I omit the value part of my expression ?

I agree that it's unclear.

I consulted the Bash Reference Manual many times in the days prior to writing this. It uses null, null string, and null value to refer to the empty string (in JavaScript parlance).

I'm attempting to convey that a user should add

version-tag-prefix =

to her .config if she wants to use X.Y.Z format rather than vX.Y.Z format. The problem with referring to the empty string is it suggests she should add

version-tag-prefix = ''

instead, which is incorrect.

Can you think of a better explanation?

I went with this:

The prefix of annotated version tags (use version-tag-prefix = for ).

⚡

Avaq · 2017-10-29T09:27:26Z

bin/test

+branches="$(get min-branch-coverage)"
+
+node_modules/.bin/istanbul cover node_modules/.bin/_mocha
+node_modules/.bin/istanbul check-coverage --branches "$branches"


Did you consider updating to nyc?

No. Do you think we should do so? What are the advantages?

Let's wait for a tool that uses V8 native code coverage reporting.

It just spawned into existence https://github.com/bcoe/c8

Avaq · 2017-10-29T09:31:53Z

bin/update-copyright-year

+fi
+
+declare -r creator="${1-Sanctuary}"
+declare -r license="${2-LICENSE}"


What's the logic which determines a script using command line arguments over a value in .config? Incidentally, could you teach me about ${1-foo}-syntax?

What's the logic which determines a script using command line arguments over a value in .config?

This script is inconsistent in this respect, but for good reasons (I believe). It seems to me the script most likely to be used in non-Sanctuary projects. It's thus important that the name of the licence file is configurable (check-required-files actually requires a file named LICENSE, so this is only relevant to projects using scripts à la carte).

I'd like to support those who wish to use only this script. It would be burdensome and a little messy to require a configuration file in order to use a single shell script.

Perhaps I'm overthinking this. It would certainly be more consistent to move these to .config, and to remove from check-required-files the hard-coded reference to LICENSE. Is this what you suggest?

Incidentally, could you teach me about ${1-foo}-syntax?

The first thing to know is that $var is shorthand for ${var}:

$ VAR=hello bash -c 'var=world ; echo "$VAR $var"' hello world $ VAR=hello bash -c 'var=world ; echo "${VAR} ${var}"' hello world

As you're no doubt aware, $1, $2, $3, etc. refer to command-line arguments. We now know that these can be written as ${1}, ${2}, ${3}, etc.

§ 3.5.3 Shell Parameter Expansion:

${parameter:-word}

If parameter is unset or null, the expansion of word is substituted. Otherwise, the value of parameter is substituted.

$ bash -c 'foo=42 ; bar= ; echo "foo: ${foo:-default}, bar: ${bar:-default}, baz: ${baz:-default}"' foo: 42, bar: default, baz: default

We are omitting the colon, though, which changes the handling of null:

When not performing substring expansion, using the form described below (e.g., ‘:-’), Bash tests for a parameter that is unset or null. Omitting the colon results in a test only for a parameter that is unset. Put another way, if the colon is included, the operator tests for both parameter’s existence and that its value is not null; if the colon is omitted, the operator tests only for existence.

$ bash -c 'foo=42 ; bar= ; echo "foo: ${foo-default}, bar: ${bar-default}, baz: ${baz-default}"' foo: 42, bar: , baz: default

So, ${1-Sanctuary} evaluates to the value of the first command-line argument if at least one command-line argument is provided; Sanctuary otherwise. As is so often the case with Bash, this convenient functionality is hidden behind arcane syntax few will ever discover.

Avaq · 2017-12-11T14:31:34Z

bin/check-required-files

+  fi
+}
+
+check -f .circleci/config.yml


It's unfortunate that this script creates a circleci lock-in. I've tried using sanctuary-script on my own project which uses Travis, and avoiding this check is not that straightforward.

Good point! Shall we simply remove this check?

That would certainly solve my issue. Perhaps we can also check that all source-files are present (not sure if we want to though), and in that spirit, we could add a dev-files options or something that defaults to .circleci/config.yml. In that case I could avoid this check by adding the following to my config:

dev-files = .travis.yml

The benefit of such a check is marginal if one must remember to enable the check. In other words, one could just as easily touch .travis.yml && git add .travis.yml as add dev-files = .travis.yml to .config.

I've removed the check above. :)

Avaq · 2017-12-15T11:02:17Z

bin/lint-readme

+  --no-stdout \
+  --use remark-lint-no-undefined-references \
+  --use remark-lint-no-unused-definitions \
+  --use validate-links \


The validate-links plugin fails all README's generated by generate-readme, because it does not take HTML heading elements into account when verifying links to anchors.

davidchambers · 2017-12-18T19:25:31Z

bin/generate-readme

+  ')"
+
+  local -r pattern1='^<h4 name="([^"]*)#([^"]*)">'
+  local -r pattern2='^(.+) ([Vv]):(.+)/([^#]+)'  # V => 1.2.3 ; v => v1.2.3


⚠️ This must be updated in accordance with sanctuary-js/sanctuary#468.

davidchambers · 2018-01-04T13:10:17Z

bin/lint-package-json

+
+assert "pkg.name === '$name' || pkg.name === '@$owner/$name'"
+
+assert "matches(pkg.version, /^[0-9]+[.][0-9]+[.][0-9]+$/)"


⚠️ This will fail for pre-release versions. We should either remove this assertion or relax the pattern.

I used /^\d+[.]\d+[.]\d+(?:-[a-z0-9.]+)?(?:[+][a-z0-9.]+)?$/ in one of my own projects.

Let's just use /./ here. All I really want to ensure is that the field is present.

The reason I've used such specific checking, is because another part of my code relies on this value having that specific format.

The same might be said about bin/release depending on the format of pkg.version, so I think it's worth considering the strict validation so to prevent release from failing in an unexpected way.

release would fail with a message such as Invalid version: XXX before performing any irreversible actions, so I don't think extra validation is warranted. Furthermore, one should rarely need to edit pkg.version manually so invalid values are unlikely.

Avaq

Ready to merge?

davidchambers requested a review from Avaq August 12, 2017 23:11

Avaq reviewed Aug 14, 2017

View reviewed changes

Avaq reviewed Aug 20, 2017

View reviewed changes

Avaq reviewed Aug 21, 2017

View reviewed changes

davidchambers mentioned this pull request Aug 31, 2017

TypeScript support sanctuary-js/sanctuary#431

Open

davidchambers force-pushed the davidchambers/everything branch from 0fbf6e4 to 64d6cac Compare October 20, 2017 15:50

davidchambers force-pushed the davidchambers/everything branch 8 times, most recently from 8ed1313 to 3d870d3 Compare October 28, 2017 11:54

Avaq reviewed Oct 29, 2017

View reviewed changes

Avaq reviewed Dec 11, 2017

View reviewed changes

Avaq reviewed Dec 15, 2017

View reviewed changes

davidchambers commented Dec 18, 2017

View reviewed changes

davidchambers commented Jan 4, 2018

View reviewed changes

davidchambers mentioned this pull request Jan 6, 2018

Update dependencies sanctuary-js/sanctuary-site#47

Merged

Avaq approved these changes Jan 6, 2018

View reviewed changes

Avaq approved these changes Jan 12, 2018

View reviewed changes

davidchambers force-pushed the davidchambers/everything branch from bca5846 to 8c4ce48 Compare January 21, 2018 16:36

Avaq mentioned this pull request Jan 22, 2018

Sort is incredibly slow sanctuary-js/sanctuary#477

Closed

everything

3f2ae5f

davidchambers force-pushed the davidchambers/everything branch from e565b47 to 3f2ae5f Compare February 4, 2018 12:56

davidchambers merged commit 5423315 into master Feb 4, 2018

davidchambers deleted the davidchambers/everything branch February 4, 2018 13:01

davidchambers mentioned this pull request Feb 4, 2018

everything sanctuary-js/sanctuary-maybe#3

Merged

		@@ -0,0 +1,3 @@
		# sanctuary-scripts

		Shell scripts used in multiple Sanctuary projects.


		## `lint-package-json`

		Asserts that __package.json__ exists and contains certain fields with


		## `lint-json`

		Asserts that the specified JSON files exist and are neatly formatted.


		assert "pkg.name === '$name' \|\| pkg.name === '@$owner/$name'"

		assert "matches(pkg.version, /^[0-9]+[.][0-9]+[.][0-9]+$/)"

everything #1

everything #1

Conversation

davidchambers commented Aug 12, 2017

Choose a reason for hiding this comment

Avaq left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avaq Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avaq Aug 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avaq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avaq Oct 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avaq left a comment

Choose a reason for hiding this comment

Avaq left a comment •

edited

Loading

Avaq Aug 14, 2017 •

edited

Loading

Avaq Aug 20, 2017 •

edited

Loading

Avaq Oct 29, 2017 •

edited

Loading