Clarify that the context root can be iterated over

[Mr0grog]

Since some implementations differ on whether the root of the context stack can be iterated over, this adds langauge to the "Sections" spec clarifying that it can. It also adds a test for this case.

Fixes #87. I did not add the two additional tests for the "Interpolation" spec that I recommended in that issue since there are tests that cover them now (not sure if these are new or if I just missed them before).

The first commit here is just the result of running rake build before making the above changes, since the current JSON and YAML are out of sync. I can separate that into a different PR if helpful, or you can just review this by focusing on the second commit. (As a side note: would it be helpful to have a GitHub action to check PRs for whether the JSON and YAML are in sync? I’m happy to do a PR that adds that.)

[jgonggrijp]

☑️ The changes look reasonable to me.
☑️ Wontache passes the new spec without modification.
☑️ I can confirm that the updated JSON is exactly the output of rake build.

Open questions:

• Is this just fixing an omission (I say yes) or do some consider this "too breaking"?
• You said the interpolation tests you suggested in Clarification on root-level implicit iterators #87 (comment) were already included. I found the second one but not the first, copied below. To me, it looks reasonable to add as well, though I think it belongs in the sections.yml.

- name: Dotted Names - Single Period
  desc: A single period should resolve to the top of the context stack.
  data:
    name: 'Alice'
  template: '{{#name}}({{.}}){{/name}}'
  expected: '(Alice)'

[jgonggrijp]

(As a side note: would it be helpful to have a GitHub action to check PRs for whether the JSON and YAML are in sync? I’m happy to do a PR that adds that.)

Yes please!

[Mr0grog]

Is this just fixing an omission (I say yes) or do some consider this "too breaking"?

From a semantic versioning and spec-based perspective, I think this is a minor release/new feature (so: v1.4.0). It is adding definition where behavior is currently undefined.

That said, I can definitely see the argument that, from an implementor perspective, this is breaking and requires a major release, since code that passes tests and matches the specs today could become incorrect as soon as this lands (e.g. Hogan would become non-compliant). But the same could be said for a lot of (most?) new features in the spec. I think the more salient thing is whether this changes existing defined behavior, and it does not do that.

You said the interpolation tests you suggested… were already included. I found the second one but not the first

Ah! I should have linked them in the PR description, sorry. They are:

• The one I called "Dotted Names - Single Period" is covered by:

  spec/specs/sections.yml

  Lines 68 to 75 in 982aa9b

  	- name: Variable test
  	desc: |
  	Non-false sections have their value at the top of context,
  	accessible as {{.}} or through the parent context. This gives
  	a simple way to display content conditionally if a variable exists.
  	data: { foo: "bar" }
  	template: '"{{#foo}}{{.}} is {{foo}}{{/foo}}"'
  	expected: '"bar is bar"'

  (This one is already in the "Sections" spec, which is good. I think you are correct that it’s a better place than where I was thinking in the "Interpolation" spec.)

• The one I called "Dotted Names - Root-level Single Period" is covered by:

  spec/specs/interpolation.yml

  Lines 202 to 208 in 982aa9b

  	- name: Implicit Iterators - Basic Interpolation
  	desc: Unadorned tags should interpolate content into the template.
  	data: "world"
  	template: |
  	Hello, {{.}}!
  	expected: |
  	Hello, world!

  (I’m not sure why this has “implicit iterators” in the name, since it’s not iterating… 🤷)

[jgonggrijp]

From a semantic versioning and spec-based perspective, I think this is a minor release/new feature (so: v1.4.0). It is adding definition where behavior is currently undefined.

This didn't sit well with me, but fortunately, the README is quite clear about what should happen:

Roughly described, major version changes will always represent backwards incompatible changes,

As you wrote, the change is not backwards incompatible, so no major version.

minor version changes will always represent new language features and will be backwards compatible,

I would argue that there is no new language feature in play here. You are just making a clarification about a language feature that was previously introduced. Hence, I don't think this is a minor version, either.

and patch ('tiny') version changes will always be bug fixes.

My interpretation of "bug" here is

1. something about the infrastructure that doesn't work;
2. having two tests (or more) that contradict each other (inconsistency label);
3. missing a test for something that was already intended during an earlier change (omission label).

My interpretation of your change is that it is an instance of number 3, so I think this change by itself would constitute a patch version.

[jgonggrijp]

@spullara or @bobthecow, do you agree we can merge this?

[spullara]

Looks good to me. I am pretty sure this is already my current behavior.

[jgonggrijp]

Thanks and congratulations @Mr0grog!

[bobthecow]

👍 to merging it, but 👎 for making it a patch release. it's specifying previously unspecified behavior, so it's at least a minor version. "bug" is "we meant a thing, but tested for it poorly".

i'd interpret the backwards compatibility requirement not as "won't make existing implementations non-compliant" but as "changing a thing previously required", i.e. it wouldn't be possible to pass both the previous version and this version of the spec. so this feels like a minor version change to me.

[jgonggrijp]

👍 to merging it, but 👎 for making it a patch release. it's specifying previously unspecified behavior, so it's at least a minor version. "bug" is "we meant a thing, but tested for it poorly".

Do you think this is previously unspecified behavior? I think this was intended all along, but tested incompletely. In #87, everyone seemed to have interpreted the spec in this way already. Just a few comments up, @Mr0grog also pointed out that similar behavior was already specified for the interpolation tag, which makes it seem reasonable that the section tag should behave analogously:

spec/specs/interpolation.yml

Lines 202 to 208 in 982aa9b

	- name: Implicit Iterators - Basic Interpolation
	desc: Unadorned tags should interpolate content into the template.
	data: "world"
	template: |
	Hello, {{.}}!
	expected: |
	Hello, world!

i'd interpret the backwards compatibility requirement not as "won't make existing implementations non-compliant" but as "changing a thing previously required", i.e. it wouldn't be possible to pass both the previous version and this version of the spec. (...)

Sound like we all agree on how to decide whether something is a major release. Perhaps you missed my previous comment: #176 (comment).

[bobthecow]

Do you think this is previously unspecified behavior? I think this was intended all along, but tested incompletely.

if that's the case, and if everyone implemented it this way, then i'm fine with "patch".

Perhaps you missed my previous comment

I didn't miss it, I was confirming that I don't think it requires a major version, as an aside, while talking about whether it was minor or patch 😛