search

openforcefield / openff-qcsubmit

Automated tools for submitting molecules to QCFractal
https://openff-qcsubmit.readthedocs.io/en/latest/index.html
MIT License
26 stars 4 forks source link

GeometricProcedure.get_optimzation_keywords is misspelled #222

Open ntBre opened 1 year ago

ntBre commented 1 year ago

Optimization is missing its second i. Link to the code. I'm happy to submit a PR if only the code needs to change. I'm not sure how to update the documentation unless it's generated straight from the source.

ntBre commented 1 year ago

Oops, I guess it's called get_optimzation_spec on the main branch and in the commit I linked to but get_optimzation_keywords on the next branch where I was working.

mattwthompson commented 1 year ago

I'm in support of changing that. If we want to be extremely delicate we can probably have an alias function with the current spelling that throws some sort of warning and routes to the correctly-spelled function, with the intent to remove it sometime. I personally would be find with breaking it as long as everything is consistent within a single version.

ntBre commented 1 year ago

I just made the changes on a branch starting from main, but now I'm wondering if we should just wait until the next branch gets merged since the method name changed anyway. It's definitely a very minor duplication to do it twice, but maybe the version on main doesn't matter right now? I'm happy with whatever, I just noticed it while working on something else.

mattwthompson commented 1 year ago

I don't think there's a plan to make a new release based off of main - I figure this change should be based off of next, or just included in that PR.

This also implies that the mega-PR shouldn't be merged, but the next branch should simply become main when it's ready to go. We did a similar thing the last time the toolkit had major breaking changes, though admittedly that was easier to pull off because it was part of a master -> main switch.

ntBre commented 1 year ago

Ahh, okay. I was thinking about the merge, so that makes sense. I'll just push it to next with my other changes. Thanks!