search

mustache / spec

The Mustache spec.
MIT License
371 stars 71 forks source link

Clarify that the context root can be iterated over #176

Closed Mr0grog closed 1 year ago

Mr0grog commented 1 year ago

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 commented 1 year ago

(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 commented 1 year ago

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:

jgonggrijp commented 1 year ago

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 commented 1 year ago

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

spullara commented 1 year ago

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

jgonggrijp commented 1 year ago

Thanks and congratulations @Mr0grog!

bobthecow commented 1 year ago

👍 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 commented 1 year ago

👍 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:

https://github.com/mustache/spec/blob/982aa9bf9d68b08f1baef049c289196e781ed2ab/specs/interpolation.yml#L202-L208

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: https://github.com/mustache/spec/pull/176#issuecomment-1813519077.

bobthecow commented 1 year ago

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 😛