search

editorconfig-checker / editorconfig-checker

A tool to verify that your files are in harmony with your .editorconfig
https://editorconfig-checker.github.io/
MIT License
437 stars 52 forks source link

Does binding the version of the binary to the version of the config still make sense? #383

Open klaernie opened 3 weeks ago

klaernie commented 3 weeks ago

In testing I've now seem multiple cases where the version stored in the binary is not set:

  1. when testcases/benchmarks are run a. … using make test b. … from an IDE integration
  2. when editorchecker-config is installed using go install ./...
  3. when the binary is built without make build

In all of these cases the response is the same:

Version from config file is not the same as the version of the binary
Binary: , Config v3.0.3

For case 1a it was easily solvable - include the right version of the -ldflags argument in the make target.

For case 1b it would be required to configure every sensible IDE integration the same way - in my personal case I would need to configure a vscode launch.json - but that would have to be repeated for each and every possible IDE.

For cases 2 and 3 every possible consumer would need to know to include the -ldflags as in go install -ldflags="…" ./.... This is not solvable in every case.

Now I'm wondering what we are trying to accomplish with that check in the first place. Reading back through the existing commits the version field in the config became necessary to specify what version of the core binary to use, irrespective of the version of the wrapper. But for the core binary itself I could not find any reason why it should reject versions.

Q1: If the only goal is to ensure that wrappers know which version to load, than no comparison in the core binary is needed. Q2: And if we still want to ensure that no old binary is trying to make sense of a new configuration, we should loosen the requirement in the core binary to do a minimum version comparison, but hardcode major version (as is done with the go import paths anyway).

This combined with our SemVer policy means that every binary can choose which config versions it supports (e.g. v4 can opt-in to supporting v3 configs, but refuse v2 configs). Initializing a new config file would remain unchanged - it would still write the version of the binary according to the best of it's knowledge into the config file (resorting to writing just the major version if that is all that is available, but e.g. v3.0.3 if the binary was built with -ldflags).

What are your thoughts, fellow @editorconfig-checker/core ?

mstruebing commented 3 weeks ago

A short and quick answer from what I remember is that it had purely to do with the wrappers. The comparison in the core was most likely to prevent from unknown/unwanted version updates in the wrappers or to prevent a falsy implementation in the wrappers (fetching the binary, etc.)

So: This should not really matter for the core, but the wrappers should be respected when removing it.

klaernie commented 3 weeks ago

Okay, so from that understanding all we need to do is hardcode the version instead of passing it in during build, but limit it to v3 instead of v3.0.3, then modify the check to strings.HasPrefix(config.Version, version).

This way we keep the intention of the check fully intact, but increase the usable window to the entire major version removing the problematic build-time injection.