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

[FR] Detect changes to exports and recommend a breaking release #439

Open
fonsp opened this issue Dec 13, 2021 · 6 comments
Open

[FR] Detect changes to exports and recommend a breaking release #439

fonsp opened this issue Dec 13, 2021 · 6 comments

Comments

@fonsp
Copy link

fonsp commented Dec 13, 2021

Some package managers (like elm-package) can automatically enforce semver by diffing public API before and after a release. In Julia, this is more complicated, but I still think we can do something in this area!

When a package changes its exports, i.e. if names(Example) changes after a release, the release must be breaking. Removing an export is breaking for obvious reasons, but adding an export can also break dependent code, because it can introduce a new implicit import clash with another package. (Thanks @pfitzseb for pointing this out.) We could detect this and show a warning, and possibly block automerge.

I recently experienced this myself (oops), and I feel like the General CI should not have allowed me to register a minor release that adds an export.

@DilumAluthge
Copy link
Member

I agree that removing an export is breaking. I would support adding a new AutoMerge check that makes sure that if a package has removed an export, then that release must be a breaking release.


Personally, I do not agree that adding a new export is breaking. Instead, I personally believe that nobody should do using Foo. Instead, do any of the following:

  1. import Foo
  2. using Foo: Foo
  3. using Foo: name1, name2, name3, ...

@GunnarFarneback
Copy link
Contributor

There are some muddying factors like the exports not necessarily defining the public API, things being explicitly marked as experimental etc.

I generally agree about import clashes being the importer's fault.

@pfitzseb
Copy link

Personally, I do not agree that adding a new export is breaking. Instead, I personally believe that nobody should do using Foo.

I agree, but there's plenty of code out there that does use using Foo, and that code may break with "non-breaking" updates adding exported bindings. So, like all of semver, this is a judgement call we have to make and should codify somewhere.

Base has added exports in 1.x releases, so I guess that's the decision. We could still add checks to auto-merge to

  • require a breaking release when removing an export
  • warn for non-breaking releases adding an exported binding

There are some muddying factors like the exports not necessarily defining the public API, things being explicitly marked as experimental etc.

Yeah, that's long been a gripe of mine... We really should find a better way to define a package's API.

@GunnarFarneback
Copy link
Contributor

Or we could check whether packages that do using Foo use tilde compat for Foo. :trollface:

@hyrodium
Copy link

There are some muddying factors like the exports not necessarily defining the public API, things being explicitly marked as experimental etc.

I think exporting experimental functions should be avoided, and adding rules not to reduce exports without breaking release would be beneficial.
Or, is there any reasonable situations to allow exporting experimental functions?

Or we could check whether packages that do using Foo use tilde compat for Foo. :trollface:

Just clarification, we should not allow tilde specifiers with major versions like PkgC = "^1".
https://pkgdocs.julialang.org/v1/compatibility/#Caret-specifiers

@hyrodium
Copy link

hyrodium commented Oct 16, 2023

How about adding rules like this?

  • vX.Y.ZvX.Y.Z+1 : Check that names(Foo) remains unchanged.
  • vX.Y.ZvX.Y+1.0 : Check that no exports are removed from names(Foo).
  • v0.X.Yv0.X.Y+1 : Check that no exports are removed from names(Foo).

We don't need to check names(Foo) for the following breaking releases.

  • vX.Y.ZvX+1.0.0
  • v0.X.Yv0.X+1.0
  • v0.0.Xv0.0.X+1

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

No branches or pull requests

5 participants