Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: use compatible instead of compatibility #3990

Closed
wants to merge 1 commit into from

Conversation

kylezs
Copy link
Contributor

@kylezs kylezs commented Sep 12, 2023

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have updated documentation where appropriate.

Summary

Realised we were using "compatibility" when "compatible" makes more sense here. Will require a small migration.

@kylezs kylezs requested a review from a team as a code owner September 12, 2023 07:03
@kylezs kylezs requested review from niklasnatter, zoheb391 and dandanlen and removed request for a team September 12, 2023 07:03
@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Merging #3990 (b31b732) into main (48e5697) will increase coverage by 0%.
The diff coverage is 58%.

@@          Coverage Diff          @@
##            main   #3990   +/-   ##
=====================================
  Coverage     72%     72%           
=====================================
  Files        366     366           
  Lines      57289   57289           
  Branches   57289   57289           
=====================================
+ Hits       41350   41355    +5     
+ Misses     13849   13845    -4     
+ Partials    2090    2089    -1     
Files Changed Coverage Δ
state-chain/custom-rpc/src/lib.rs 0% <0%> (ø)
state-chain/pallets/cf-environment/src/weights.rs 0% <0%> (ø)
state-chain/runtime/src/lib.rs 39% <0%> (ø)
state-chain/traits/src/lib.rs 53% <ø> (ø)
state-chain/pallets/cf-environment/src/lib.rs 71% <25%> (ø)
state-chain/pallets/cf-governance/src/mock.rs 97% <50%> (ø)
...e-chain/pallets/cf-environment/src/benchmarking.rs 90% <100%> (ø)
state-chain/pallets/cf-environment/src/mock.rs 70% <100%> (ø)
state-chain/pallets/cf-environment/src/tests.rs 100% <100%> (ø)
state-chain/pallets/cf-governance/src/lib.rs 69% <100%> (ø)
... and 1 more

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@dandanlen dandanlen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why the naming seemed to make sense as it is, is that this is a "version that indicates api compatibility", hence compatibility version. "Compatible version" would seem to suggest that it's the only compatible version, which is not the case (you might have a different version which is nonetheless compatible).

Current naming is not perfect but fits well enough. If we can find something less confusing we can rename, but I'm not convinced compatible_version is much clearer.

Either way, if we change it, I would rather we include the storage migration in this PR though, so as not to forget it.

@kylezs
Copy link
Contributor Author

kylezs commented Sep 13, 2023

version that indicates api compatibility

aka "compatible version" in basically all other software that adheres to standard english naming.

I don't see how "compatibility version" indicates that it could be more compatible versions... it's still the singular version, just written in non-standard english - making it harder to remember and find.

I don't think because we have a particular versioning system we should discard english... I did struggle to find this when I was looking because I was searching for "compatible"

I would rather we include the storage migration in this PR though

I can add the migration here if you're happy with the change.

@dandanlen
Copy link
Collaborator

I don't know what you mean about non-standard English. Compatibility is a noun, compatible is an adjective.

I'll try to explain myself again. CompatibleVersion, to me (and I do consider myself a native speaker), indicates that this is some version identifier that has the property 'compatible'. This doesn't feel right - two version can be compatible with each other. But a version on its own can't be universally compatible. What is it compatible with? Everything?

What we actually want to say: this is the FooVersion that the CFE needs to compare its own FooVersion against in order to determine if CFE and Runtime are compatible.

We want to give it a name/qualifier Foo to distinguish it from the release version, the runtime version, api version, whatever else (and note all of these qualifiers are nouns, not adjectives). It would be nice to find a suitable word that captures this 'Foo', and I admit 'Compatibility' seems like an unfortunate choice (not least because it's somewhat redundant: what else does a version denote but compatibility with something?), but the problem here is not that we chose a noun instead of an adjective.

Like I said, I can see how it's currently (evidently) confusing, but I don't think this PR resolves any of that confusion.

@msgmaxim
Copy link
Contributor

My two cents as a non-native speaker. We don't talk about runtime being compatible with the CFE, but rather whether the CFE is compatible with the runtime. Calling the version we put on SC "compatible" feels a bit wrong to me. It is the version that we use to check CFE's compatiblility against, and so (while it is not perfect) it does make slightly more sense. It doesn't feel too different something like "compatibility criteria", for example, which we do use in english.

@kylezs kylezs closed this Sep 19, 2023
@kylezs kylezs deleted the chore/compatible-compatibility branch September 19, 2023 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants