libnano / primer3-py

Simple oligo analysis and primer design
https://libnano.github.io/primer3-py
GNU General Public License v2.0
157 stars 43 forks source link

migrated primer3 header file Cython extern imports to `thermoanalysis.pxi` #100

Closed grinner closed 1 year ago

grinner commented 1 year ago

To enable cleaner third party use of primer3-py in cython applications, it's useful to remove header file imports.

This was accomplished with "shadow" structs for:

And by converting pointer attributes to `void*

This change, and using the appropriate type casts in thermoanalysis.pyx enabled a move of all extern header imports to a new file thermoanalysis.pxi

It is now cleaner to make a from primer3.thermoanalysis cimport _ThermoAnalysis call in 3rd party software with no need for any header files from the primer3 C library

NOTE: breaking Cython API change: Optional C structure string argument c_ascii_structure added to _ThermoAnalysis methods to enable 3rd party use for structures

Version bump to 2.0.0a1 due to breaking change and 1.3.0-staging branch has been renamed to 2.0.0-staging

Follow up PR will update documentation to better describe Cython integration of this code

grinner commented 1 year ago

Changes look good. Wouldn't this constitute a breaking change (and thus warrant a major version bump)? I don't think we've formally defined what constitutes the "external API" for this project (for the purposes of semantic versioning) but I'd imagine that the migration from pxd to pxi alone would qualify as a breaking change.

I think you're right that technically speaking this is a breaking change.
It would require a version bump. Since this is committing to 1.3.0-staging I don't see a problem in merging as is, but perhaps a good guard would be a bump to 2.0.0a1 in the __version__ string

Additionally, I think the 2.0.0 release should get a documentation update for external use with Cython.

benpruitt commented 1 year ago

Agree re: version string bump -- if you want to do that then I think this is ready for approval (and maybe just push up a 2.0.0-staging branch and delete the 1.3.0-staging branch?

grinner commented 1 year ago

Did the branch rename to 2.0.0-staging and changed the version string to '2.0.0a1'

Once documentation is updated for Cython instructions, we can do the 2.0.0 release

benpruitt commented 1 year ago

@grinner do you want to update the documentation in this PR or in a subsequent PR? We should also discuss whether it makes sense to include anything else in the 2.0.0 scope (and the associated timing).

grinner commented 1 year ago

@grinner do you want to update the documentation in this PR or in a subsequent PR? We should also discuss whether it makes sense to include anything else in the 2.0.0 scope (and the associated timing).

@benpruitt Updating documentation in a follow up PR prior to 2.0.0 merge