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

renamed pdb_delinsertion #92

Merged
merged 12 commits into from
Mar 7, 2021
Merged

Conversation

joaomcteixeira
Copy link
Member

pdb_delinsertion was renamed to pdb_renuminsertion.

Closes #91

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

`delinsertion` was renamed to `renuminsertion`.
@codecov
Copy link

codecov bot commented Jan 7, 2021

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%) ⬆️

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.

Copy link
Member

@amjjbonvin amjjbonvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty straightforward. So green light from my side if you all think it is a good idea.

@joaomcteixeira
Copy link
Member Author

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
Copy link
Member

amjjbonvin commented Jan 9, 2021 via email

@JoaoRodrigues
Copy link
Member

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
Copy link
Member

amjjbonvin commented Jan 14, 2021 via email

@JoaoRodrigues
Copy link
Member

JoaoRodrigues commented Jan 28, 2021

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 JoaoRodrigues removed their request for review January 28, 2021 02:43
@JoaoRodrigues
Copy link
Member

If you could review @joaomcteixeira that would be good!

@joaomcteixeira
Copy link
Member Author

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:

  • there is another big change that we cannot skip a MAJOR upgrade, so we use it to remove the deprecated ones
  • we accumulate a certain number of deprecations
  • we decide every year at a certain date we release a new MAJOR upgrade if there are any deprecations
  • we assume there is no big infrastructure depending on minor releases of pdb-tools (I am not considering HADDOCK here obviously) and the user community are actually human users so we just bump the major version this very same PR and keep the pdb-tools development agile. If if there is a major infrastructure depending on pdb-tools, it is not our fault if they don't constrained the dependencies to version 2 =<2.

I am happy with any of those.

@JoaoRodrigues
Copy link
Member

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
Copy link
Member Author

joaomcteixeira commented Jan 28, 2021

Yes, that is what I meant in the docstring 👍

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 😉 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.

  • with the aggressive strategy you explicitly state to users your dev/dep strategy
  • With the latter strategy, you give users time to adapt and predictable expectations of stability

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

@JoaoRodrigues JoaoRodrigues merged commit 15f3341 into haddocking:master Mar 7, 2021
@joaomcteixeira joaomcteixeira deleted the issue91 branch March 7, 2021 23:16
@amjjbonvin
Copy link
Member

amjjbonvin commented Mar 8, 2021 via email

@joaomcteixeira
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pdb_delinsertion - naming is confusing
3 participants