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

Does the App follow semantic versioning? #119

Open
martin-rueegg opened this issue Aug 3, 2022 · 3 comments
Open

Does the App follow semantic versioning? #119

martin-rueegg opened this issue Aug 3, 2022 · 3 comments

Comments

@martin-rueegg
Copy link

Workflow Script app

Workflow Script app version: 1.9.0

44f73ab seems to introduce several breaking changes.

E.g. in Launcher.php the signature of OCA\WorkflowScript\BackgroundJobs\Launcher's constructor changes from

public function __construct(LoggerInterface $logger, ITempManager $tempManager, IRootFolder $rootFolder)

to

public function __construct(ITimeFactory $time, LoggerInterface $logger)

Similarly affected is OCA\WorkflowScript\Operation and more subtly OCA\WorkflowScript\Listener\RegisterFlowOperationsListener where only the interface of the parameter changes.

Do these changes not violate the backwards-compatibility requirement, that is implied by the same major version?

Hence, should version 1.9.0 not have been 2.0.0 instead?

The whole idea of semantic versioning is to avoid code breaks on dependencies. According to the (developer documentation)[https://nextcloudappstore.readthedocs.io/en/latest/developer.html#info-xml], version numbers must follow semantic versioning.

@joshtrichards
Copy link
Member

Hi @martin-rueegg

There may be room for improvement, but - unless I'm mistaken - the things you mentioned above aren't what I'd call public facing APIs.

Did you encounter a problem or some unexpected behavior in your use case that prompted this Issue?

There's obviously some judgement involved so maybe something got overlooked.

The docs you linked to are mostly for external developers. The following is for NC developed apps and the versioning management is more explicitly defined there:

https://docs.nextcloud.com/server/latest/developer_manual/app_publishing_maintenance/release_process.html#versioning

@martin-rueegg
Copy link
Author

Hi @joshtrichards

Thank you for getting back at me. Well, I was looking into implementing a picture and video resizing app, that is currently implemented as an external script solution, into an app for nextcloud and hence bilding on top of the Workflow Script.

However, in extending existing classes, it is useful to know, how stable an API is and how breaking changes are announced. Hence the question.

It may be, that for NC environment the semver scheme mainly focuses on the environment an app is running in (this is implied by the documentation of your above mentioned documentation). However, technically, every non-private method is part of an API. And I personally consider a change to a public constructor to be a (breaking) change of the API and hence reflect that in the version number. Even in my internal libraries. Just to make sure a change doesn't get missed. And also for an extension to easier detect which version of a method is to be called.

As such, I would encourage this for any app that implies semver. As otherwise the whole point is lost - if breaking changes are only defined by the compatibility with the environment and not also with extensions.

That are just my 2 cents. :-)

@blizzz
Copy link
Member

blizzz commented Nov 15, 2023

Hey @martin-rueegg,

this app does not expose any API and any classes that are implemented here are considered to internal.

We also do (try to) avoid inter-app dependencies. For use cases that require apps communicating with each other, APIs in the server shall be defined, or Event classes in either the server or the app.

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

3 participants