haddocking / pdb-tools

A dependency-free cross-platform swiss army knife for PDB files.
https://haddocking.github.io/pdb-tools/
Apache License 2.0
372 stars 113 forks source link

renamed pdb_delinsertion #92

Closed joaomcteixeira closed 3 years ago

joaomcteixeira commented 3 years ago

pdb_delinsertion was renamed to pdb_renuminsertion.

Closes #91

:warning: @JoaoRodrigues @amjjbonvin This change should upgrade pdb-tools to version 3 by definition of SV2. This PR breaks backward compatibility.

codecov[bot] commented 3 years ago

Codecov Report

Merging #92 (bc2802b) into master (3dbfcbb) will increase coverage by 0.07%. The diff coverage is 88.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #92      +/-   ##
==========================================
+ Coverage   82.00%   82.08%   +0.07%     
==========================================
  Files          46       47       +1     
  Lines        3663     3679      +16     
  Branches      763      763              
==========================================
+ Hits         3004     3020      +16     
  Misses        469      469              
  Partials      190      190              
Impacted Files Coverage Δ
pdbtools/pdb_fixinsert.py 87.50% <87.50%> (ø)
pdbtools/pdb_delinsertion.py 100.00% <100.00%> (+12.63%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3dbfcbb...bc2802b. Read the comment docs.

joaomcteixeira commented 3 years ago

Perfect. I am fine with both ways also. If you prefer not to increase the major version we can duplicate the client so as to maintain both versions and backward compatibility. I don't know if delinsertion is being used in any HADDOCK pipeline, or alike. From the individual user point of view, I believe won't be a problem to increase the version number. We should keep a CHANGE.log though.

What is your opinion @JoaoRodrigues ?

amjjbonvin commented 3 years ago

We will change the server as needed if the name changes

JoaoRodrigues commented 3 years ago

I see what you mean by "delete". The idea here is that the tools delete insertion codes, thus the renumbering. I personally find pdb_renuminsertion really long (and also not necessarily correct because we do delete something). What about pdb_fixinsert or pdb_fixicode? We should also introduce a tool that does remove insertions then.

As for semantic versioning etc, I would bump the minor version because we would add a new tool (whatever the name) and then add a deprecation warning on the current pdb_delinsertion saying it will change/be removed in the next release.

amjjbonvin commented 3 years ago

My vote goes then to pdb_fixinsert

JoaoRodrigues commented 3 years ago

Changed name. We should work on the other tool then, this doesn't cover all use cases!

I added an error message to the old pdb_delinsertion tool warning users that it will be deprecated. We should retire it in the near future, but maybe not in the next release. We don't have a deprecation policy, but I'd say waiting 2 releases until it's gone sounds good. Objections?

JoaoRodrigues commented 3 years ago

If you could review @joaomcteixeira that would be good!

joaomcteixeira commented 3 years ago

If the tool starts with fix, having the docstring starting with Delete is not a good match. This happened already before in another tool. Just change the Delete to Fix in the first sentence of the docstring and then go on explaining in the following lines.

What do you mean by "next two releases?" Drop it in pdb-tools v4 ?

You cannot drop an interface after X minor (feature) releases. We can decide, though, to increase the major version number after we accumulate a certain number of deprecation warnings. Until then, if you decide the maintain the delinsert with the warning, it should remain there regardless of the number of minor upgrades.

Here are three possible deprecation policies. We keep deprecates until one of the following happens:

I am happy with any of those.

JoaoRodrigues commented 3 years ago

If the tool starts with fix, having the docstring starting with Delete is not a good match. This happened already before in another tool. Just change the Delete to Fix in the first sentence of the docstring and then go on explaining in the following lines.

What is fix? :) We should be more explicit in the docstring! Happy to have fix in the first line but have a better description in the paragraph below.

As for deprecation, I thought we could do a rule like, N minor version releases (2.4, 2.5, ...) and then bundle the deprecation (effectively remove the tool) in now + N. My idea was to bump the minor version now since we "add" a tool anyway.

joaomcteixeira commented 3 years ago

Yes, that is what I meant in the docstring :+1:

Your deprecation strategy is also very possible. But just think about the hypothetical case where the following N minor releases are also bundled to deprecations. Then, where and when you decide to set the deprecation timer for a major release becomes not obvious. Maybe all these thoughts don't apply to pdb-tools, but I know I like to think in abstract and general terms :wink: Also, if suddenly we add two more CLIs, we will be forced to drop de deprecated and bump the major maybe in a very short period of time. Or, the deprecation warning may remain there forever if it happens no more CLIs are added for the next four years. Hence, this policy may originate random events and there is no predictable rule for users to embrace.

Considering the specific case of pdb-tools, considering its size and user community, the maintainability capacity, and because @amjjbonvin already said there would not be a problem to update on the haddock side, I would just bump the major and keep the development/release agile. If this sounds too aggressive, I would send a message to the user community saying that every first of each year all deprecated clients will be removed and the major version upgraded - if there are deprecation warnings.

I will review the code later today. I don't want to approve it without testing it locally a bit. Have a meeting now :wink:

amjjbonvin commented 3 years ago

May-be good to add in the documentation what was the old name so that users can find it.

joaomcteixeira commented 3 years ago

Documentation was updated in #97

But neither of the tools delinsertion and fixinsert is appearing on the webpage. We will look to correct that.

When running pdb_delinsertion the user receives a deprecation warning and the docstring of the new tool is shown instead. pdb_delinsertion is now just bypass to pdb_fixinsert.