Lightmatter / welkin-health

A Python wrapper of the Welkin Health API
https://welkin.readthedocs.io
Other
4 stars 3 forks source link

Feature/document types #75

Closed samamorgan closed 1 year ago

samamorgan commented 1 year ago

Note: This is a breaking change. I have bumped the minor version to indicate that this is not backwards-compatible. Notably the separation of Formation classes and renaming a few inappropriately-named classes breaks compatibility with previous versions.

gone commented 1 year ago

It looks good to me - I think you should do it as a major version vs minor since semver generally says breaking changes are major releases.

I also think, from looking at all the formation classes, you might want to implement them with something like a standardized get call and a self.url format string that gets used, since that would shave off like 30/40 lines of boilerplate

samamorgan commented 1 year ago

It looks good to me - I think you should do it as a major version vs minor since semver generally says breaking changes are major releases.

I'm specifically not doing this because to me a major version indicates that the product is out of beta. This library is not yet feature-complete, does not implement all documented Welkin endpoints. From semver docs:

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable. ... Version 1.0.0 defines the public API. The way in which the version number is incremented after this release is dependent on this public API and how it changes.

I also think, from looking at all the formation classes, you might want to implement them with something like a standardized get call and a self.url format string that gets used, since that would shave off like 30/40 lines of boilerplate

I'll give this a shot. I've been avoiding that in general since it does in some ways reduce extensibility, but for these it may be appropriate.