toml-lang / toml.io

Source Code for toml.io
https://toml.io
MIT License
42 stars 44 forks source link

Update ports in specs #37

Closed ilyakam closed 3 years ago

ilyakam commented 3 years ago

It doesn't make sense to have the same port 8001 appear twice in the array. I've updated all of the specs with the ports array to be [ 8000, 8001, 8002 ], presumably as it was originally intended.

eksortso commented 3 years ago

That ought to be changed at toml-lang/toml, because that array came from the spec. But this is a good thing to do.

I wouldn't change the old specs though. What's done is done. But let's fix it going forward.

ilyakam commented 3 years ago

That ought to be changed at toml-lang/toml, because that array came from the spec. But this is a good thing to do.

Thanks! The only place where I found a reference to 8001 in the https://github.com/toml-lang/toml/ codebase is on line 34 of the README:

https://github.com/toml-lang/toml/blob/dc83e698ceecf8a3bb4b1622860a3c11febfa997/README.md#L34

If that's the one, I'll be happy to submit another PR shortly.

I wouldn't change the old specs though. What's done is done.

I humbly disagree with this statement. Before submitting this PR, I debated whether it should be just for the current spec or for all versions. I ultimately chose the latter because these changes don't affect the spec itself. Rather, they affect its implementation examples. So as long as you could define an array in TOML, the examples for ports should not contain any unexpected values in it. Or, put differently, suppose there was a typo in the user-facing copy of the examples that doesn't affect the spec. Would you or wouldn't you retroactively fix it when you discovered it?

Having said that, this issue is relatively minor, and since I'm just a guest contributor, I'll defer to your better judgement. Please let me know how you'd like me to proceed.

cannikin commented 3 years ago

I'm not sure what our policy is around old versions of the spec. Are they set in stone, and any changes must be made in a new release? Or does that only apply to actual recommendations/requirements of the spec, and code samples are changeable?

eksortso commented 3 years ago

@cannikin This is probably new territory for the standard. The decision needs to be made. Not by me though. By our elders, the project leads.

My instinct tells me to keep old text static, partly because it's still legal for the standards even if the ports arrays in past versions have duplicates and may be missing common values found in practically all use cases. Others may disagree, but in any case, I want to see where the fault tolerance lines normally get drawn.

@ilyakam Thanks for your contribution. You're certainly helping to improve things. It's good what you've done so far, and we're glad to have you here.

ilyakam commented 3 years ago

If I could offer you one more argument in favor of retroactively updating the docs, it's this: The main project repo seems to be adhering to the standards set forth by Semantic Versioning and Keep a Changelog. Semantic Versioning offers clear rules when to bump the version, the most granular of which says "PATCH version when you make backwards compatible bug fixes." Given that these retroactive changes don't even fix a bug, there's no need to go back to each minor version, like 0.1.0 and bump it to 0.1.1 when the example in its README is updated. Similarly, in its FAQ section, Keep a Changelog tells us that it's okay to rewrite the CHANGELOG: "There are always good reasons to improve a changelog." Therefore, if the version doesn't need to be bumped and it's good enough for the CHANGELOG, it should be plenty good for the documentation examples.

But again, as a guest contributor, I'll defer to your judgement on this matter. 😃

eksortso commented 3 years ago

@ilyakam With all due respect, it's clear that Semantic Versioning requires us to change the version, even if the change being proposed has no practical effect on the specification, because the README is still part of the release. Point 3 makes this explicit. What's done been released, is done.

We could proceed with any change that's not tied to a specific release number. The example on the home page could be modified independently, if it's not composed from the example on the README. (I haven't checked if @pradyunsg's release script does that.)

But please open a PR in the TOML standard project and correct the port numbers there, and be assured (and I'll vouch for you) that the correction will show up in v1.0.1 at the earliest, and will propagate to this site as part of the regular release process.

ilyakam commented 3 years ago

Thank you for pointing out to the explicit reason why you're against a retroactive change, @eksortso. It makes sense and I appreciate the feedback. I went ahead and opened a new PR on the TOML standard project; you may find it here: https://github.com/toml-lang/toml/pull/811. This PR is now closed.