[RFC] api: Nvim version + API level #5535

Merged
merged 2 commits into from Oct 28, 2016

Projects

None yet

5 participants

@justinmk
Member
justinmk commented Oct 26, 2016 edited

Note: I'm leaving out the global "prerelease" flag for 0.1.6, becaues it does not make sense to me: Any unreleased Nvim version is a "prerelease" by definition, so clients could check for a known released Nvim version (version.major == 0 && version.minor == 1 && version.patch <= 5).

After this PR, api_info()['version'] returns:

   api_info()['version'] => {
      major: 0,
      minor: 1,
      patch: 6,
      api_level: 1,
      api_compatible: 0,
      api_prerelease: false,
    }
@justinmk Rui Abreu Ferreira api: Nvim version, API level #5386
The API level is disconnected from the NVIM version. The API metadata
holds the current API level, and the lowest backwards-compatible level
supported by this instance.

Release 0.1.6 will be the first release reporting the Nvim version and
API level.

    metadata['version'] = {
      major: 0,
      minor: 1,
      patch: 6,
      prerelease: true,
      api_level: 1,
      api_compatible: 0,
    }

The API level may remain unchanged across Neovim releases if the API has
not changed.

When changing the API the CMake variable NVIM_API_PRERELEASE is set to
true, and  NVIM_API_CURRENT/NVIM_API_COMPATIBILITY are incremented
accordingly.

The functional tests check the API table against fixtures of past
versions of Neovim. It compares all the functions in the old table with
the new one, it does ignore some metadata attributes that do not alter
the function signature or were removed since 0.1.5.  Currently the only
fixture is 0.mpack, generated from Neovim 0.1.5 with nvim --api-info.
f25797f
@bfredl
Member
bfredl commented Oct 26, 2016 edited

Any unreleased Nvim version is a "prerelease" by definition, so clients could check for a known released Nvim version (version.major == 0 && version.minor == 1 && version.patch <= 5).

Not true. If the client compares against 0.1.6 after it is released, it cannot know if the binary is a prerelease for 0.1.6 or the actually released 0.1.6 . But what would correspond to my proposal in #5386 would not be this, rather api_prerelease, which is not equivalent to nvim being a prelelease, but would simply tell if the highest advertised api level is a stable level or not. Which, similarly, cannot be figured out by "just knowing" what the latest stable release of the project is; it is a property of the binary only.

@justinmk
Member

Why can't they compare a known API level?

@bfredl
Member
bfredl commented Oct 26, 2016

How could they? if api_level is 2, how could they know this means final released 0.1.7 (or 0.2 maybe) with the final api or any prerelease after api_level was first bumped to 2 some time after 0.1.6, which can span a wide range of master versions?

@justinmk
Member

They're going to need to compare the API level in any case. Because after we would set api_prerelease to false, then suddenly whatever check the client was doing, changes.

@justinmk
Member

They can check a known API level with the current API level. If the current level is something greater than expected, that communicates the same information as if prerelease were true.

@marvim marvim added the WIP label Oct 26, 2016
@justinmk
Member

How could they? if api_level is 2, how could they know this means final released 0.1.7

Sure, that decision isn't known until 0.1.7. But I am not seeing how knowing that programmatically will be of any use. For an old client the result of api_prerelease will toggle between Nvim releases!

@bfredl
Member
bfredl commented Oct 26, 2016 edited

No it doesn't. If the client prefers to always use the latest stable version, and see 2-pre, the client would use the 1 (thus stable) api. With your proposal that is impossible. The binary is a different state relative the actual stable 2 api when it is 2-pre or 2-stable, and this flags communicates exactly that. This could only be recovered by deep metadata inspection, and (part of) the point of having these global fields is to able plugins to make runtime decisions (between an older and a new supported api version) without metadata inspection. Some plugins might prefer to treat 2-pre as 2 ("it might break, try latest master and open issues") and others as 1 (always stable)

@bfredl
Member
bfredl commented Oct 26, 2016

But I am not seeing how knowing that programmatically will be of any use.

One can only see this if one accepts that different plugins might adopt different strategies vis-a-vis stability. If one says "I think in way X and expect all plugins to think in one single way X" which was done 2-3 times in #5386 (with different X), they I agree it seems worthless, because then we could just hardcode X in api_level.

For an old client the result of api_prerelease will toggle between Nvim releases!

I don't follow? Again, it provides the status relative the api level.

@justinmk
Member

Perhaps it helps to mention that I was thinking we should bump API level each time we touch the API? I.e. even before a release.

@bfredl
Member
bfredl commented Oct 26, 2016 edited

Do you mean multiple bumps between releases? Every PR that adds some functions should bump it again? [I don't think I agree to that, that would to me imply backwards compatibility also to intermediate master versions, which I think is too strong, but I understand in that situation api_prerelease is superfluous].

@jamessan
Member

Perhaps it helps to mention that I was thinking we should bump API level each time we touch the API? I.e. even before a release.

That seems like unnecessary churn. The API isn't committed to until it's released. To summarize what I thought we had agreed on in the previous discussion, this is the workflow I was expecting. All numbers are made up to protect the innocent.

Stage Version API level prerelease flag
Release 1.2.3 1 false
Hack 1.2.4-dev 1 false
Commit API change 1.2.4-dev 2 true
Hack 1.2.4-dev 2 true
Commit API change 1.2.4-dev 2 true
Release 1.2.4 2 false
Hack 1.2.5-dev 2 false
Release 1.2.5 2 false

The prerelease flag is a way to let API consumers decide whether they want to support unreleased APIs. If a consumer develops something against the released API level 2 and then a user runs one of the prerelease versions of API level 2, the consumer has the ability to know that and decide not to use the code paths that would leverage APIs from level 2, since they may not be compatible with what was actually released (which is what the consumer developed against).

Yes, you can get the same effect by bumping the API level every time the API changes, but I agree with @bfredl that doing so implies those intermediate API levels between releases are supported and shouldn't have incompatible changes between them. We want the freedom to change a newly developed API before making an official release.

If you're testing out our unreleased API, then there's the expectation that you are following the project closely and will be aware of changes to the unreleased APIs. Hopefully, you're actually providing feedback to help ensure the API works well when it is released. I don't see a need to take specific steps to support this use case.

@bfredl
Member
bfredl commented Oct 26, 2016

What @jamessan said.

Normally, we shouldn't (and we hadn't so far) modify apis that being brewing on master for a long time unless there is a really good reason. But the formal promise we make is only not breaking api:s from released versions, and the api level scheme should codify that.

@justinmk
Member

Ok, I'm on board. As an aside though, API level seems redundant in that scenario: it will probably be bumped at every release...

I'll push changes later today.

@jamessan
Member

As an aside though, API level seems redundant in that scenario

I agree. It can all be tied to the project version as long as we publish what project version a certain API was added in. However, there had been other discussion specifically about having distinct versions for the API and the project. All of this is based on that separation of versioning.

@justinmk
Member

I'll stick to the plan. Thanks both for the feedback.

@bfredl
Member
bfredl commented Oct 26, 2016

The motivation for introducing separate API levels was never about being track additions to the API independent of the nvim version to start with (neither faster nor slower), it was first for tracking deprecations and backwards compatibility (and the lack there of).
It makes it possible to say things like "nvim 1.0 supports api levels 1-3 but not 0" and "nvim 1.0 and plugin X v0.3 communicate using api level 2" rather than "nvim 1.0 and plugin X v0.3 communicate using the api as it was released in nvim 0.2".

@bfredl bfredl referenced this pull request Oct 26, 2016
Merged

[RDY] Incsub 3 #5226

@justinmk
Member

Updated.

@justinmk justinmk changed the title from [WIP] api: Report Nvim version + API level to [RFC] api: Report Nvim version + API level Oct 27, 2016
@marvim marvim added RFC and removed WIP labels Oct 27, 2016
@justinmk
Member

Would like to hear from @equalsraf , then will merge this. Perhaps other client/plugin authors could comment as well.

@justinmk justinmk changed the title from [RFC] api: Report Nvim version + API level to [RFC] api: Nvim version + API level Oct 27, 2016
CMakeLists.txt
+# API level
+set(NVIM_API_LEVEL 1) # Bump this after any API change.
+set(NVIM_API_LEVEL_COMPAT 0) # Adjust this after a _breaking_ API change.
+set(NVIM_API_PRERELEASE false)
@equalsraf
equalsraf Oct 28, 2016 Contributor

shoulnt prerelease be true right now?

i.e. if 0.1.6 is api:1 then we are in api:1 prerelease:true right now

@justinmk
justinmk Oct 28, 2016 Member

Sure. But this will be tagged for release after we merge it :)

@equalsraf
equalsraf Oct 28, 2016 Contributor

Nothing like putting release.sh to the test.

@justinmk
Member

updated

@justinmk justinmk api: api_info()['version']
API level is disconnected from NVIM version. The API metadata holds the
current API level, and the lowest backwards-compatible level supported
by this instance.

Release 0.1.6 is the first release that reports the Nvim version and API
level.

    metadata['version'] = {
      major: 0,
      minor: 1,
      patch: 6,
      api_level: 1,
      api_compatible: 0,
      api_prerelease: false,
    }

The API level may remain unchanged across Nvim releases if the API has
not changed.

When changing the API,
    - set NVIM_API_PRERELEASE to true
    - increment NVIM_API_LEVEL (at most once per Nvim version)
    - adjust NVIM_API_LEVEL_COMPAT if backwards-compatibility was broken

api_level_0.mpack was generated from Nvim 0.1.5 with:
    nvim --api-info
c5f5f42
@justinmk justinmk merged commit 5b514b5 into neovim:master Oct 28, 2016

2 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
coverage/coveralls Coverage pending from Coveralls.io
Details
QuickBuild Build pr-5535 finished with status SUCCESSFUL
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@justinmk justinmk deleted the justinmk:api_level branch Oct 28, 2016
@justinmk justinmk removed the RFC label Oct 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment