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

Remove deprecated variation-related methods #441

Closed
wants to merge 2 commits into from

Conversation

tgrego
Copy link
Contributor

@tgrego tgrego commented Nov 11, 2019

Description

A few deprecated methods have been deprecated and are due to be removed at release 95.
Those should be removed effective of release 100.
The extra 5 release should be plenty of time for any users to have updated their code.

Use case

See ENSCORESW-3294

Benefits

Less deprecation notices and runtime of our test harness.

Possible Drawbacks

Users still relying on those methods have been getting deprecation notices for a long time, if they did not update their code they will have to do so now.

Testing

Have you added/modified unit tests to test the changes?

yes

If so, do the tests pass/fail?

pass

Have you run the entire test suite and no regression was detected?

yes

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.006%) to 81.448% when pulling cfca6b8 on variation_deprecations into c226c1d on master.

Copy link
Contributor

@ens-bwalts ens-bwalts left a comment

Choose a reason for hiding this comment

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

These methods not called from anywhere else in Ensembl. All 12 calls in the test (and the variation adaptor) removed. Wondering if it would make sense to move the lonely get_all_VariationFeatures test into sliceAdaptor.t and removing sliceVariation.t entirely - not enough to request changes but something to consider.

Also agree with GitHub's suggestion that the other reviewer on this be someone from the variation team if they're willing.

Edit: Should have been more clear that I only checked the core repo - ambiguity between Ensembl (the project) and ensembl (the repo). Apologies/

@tgrego tgrego requested a review from at7 November 20, 2019 12:27
Copy link
Contributor

@at7 at7 left a comment

Choose a reason for hiding this comment

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

We cannot remove those methods from the core API. The webcode API depends on them. They are used here: https://github.com/Ensembl/ensembl-webcode/blob/release/98/modules/EnsEMBL/Web/Query/GlyphSet/Variation.pm#L205-L209
The slice used in the webcode is a compara AlignSlice. If I rewrite the method to be used in the VariationFeatureAdaptor and pass the AlignSlice from the webcode the method doesn't work.

I have never found a solution for this. Do you have an idea? If not I can remove the deprecation message.

@ens-bwalts
Copy link
Contributor

Should have been more clear that I only checked the core repo - ambiguity between Ensembl (the project) and ensembl (the repo). Apologies.

@at7
Copy link
Contributor

at7 commented Nov 20, 2019

I should have removed the deprecation message when I first found out about the compara/web dependency. I don't know why I didn't. Probably because I was hoping to come up with a solution which until today I haven't.

@tgrego
Copy link
Contributor Author

tgrego commented Nov 20, 2019

good thing we asked variation!
I'll close this pull request and we'll think of the next move

@tgrego tgrego closed this Nov 20, 2019
@magaliruffier magaliruffier deleted the variation_deprecations branch June 10, 2020 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants