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

Possibly unused QueryBuilder code #3518

Closed
CasperWA opened this issue Nov 7, 2019 · 4 comments · Fixed by #3526
Closed

Possibly unused QueryBuilder code #3518

CasperWA opened this issue Nov 7, 2019 · 4 comments · Fixed by #3526

Comments

@CasperWA
Copy link
Contributor

CasperWA commented Nov 7, 2019

Poking around in the front-end QueryBuilder code, it seems a function is never used.
I found this since it also calls the now obsolete attribute self._imp in it.

The function is except_if_input_to and can be found here.
A search through the whole repository for the function name only reveals a single hit - the definition of it.

@lekah is this function a leftover from a previous rewrite of the class, or the beginning of an addition to the QueryBuilder? And are there more of these dead-code-methods around that either need a purpose in a grander scheme or should simply be sent to oblivion?

@lekah
Copy link
Contributor

lekah commented Nov 7, 2019

Is this function a leftover from a previous rewrite of the class, or the beginning of an addition to the QueryBuilder?

This function was part of some utility, in this specific case it would remove all results that fit the criterion of being used as an input to some Calc class. I think it could still be quickly fixed to do this. But I would suggest to remove it, to avoid there being more functionality than maintenance. Also, it seems that there was no test for this which is my fault.

Are there more of these dead-code-methods around that either need a purpose in a grander scheme or should simply be sent to oblivion?

¯_(ツ)_/¯

@ltalirz
Copy link
Member

ltalirz commented Nov 7, 2019

Are there more of these dead-code-methods around that either need a purpose in a grander scheme or should simply be sent to oblivion?

There definitely are - I just stumbled upon another one in #3522

The QueryBuilder is one of the worst offenders in terms of uncovered lines in a file (it is also a rather long file..).
Here they are: https://coveralls.io/builds/26743225/source?filename=aiida/orm/querybuilder.py

@CasperWA
Copy link
Contributor Author

CasperWA commented Nov 8, 2019

Are there more of these dead-code-methods around that either need a purpose in a grander scheme or should simply be sent to oblivion?

There definitely are - I just stumbled upon another one in #3522

The QueryBuilder is one of the worst offenders in terms of uncovered lines in a file (it is also a rather long file..).

I guess most of these dead functions/methods have been lost in translation, i.e., left unused when updating the QueryBuilder to the new ORM (or similar).
The question is: Will it be worth it to update the dead functions/methods to work again (and use them where applicable) or is it easier to simply cut it all off and later re-build it if a specific function or method is ever needed in the future?

@CasperWA
Copy link
Contributor Author

CasperWA commented Nov 8, 2019

This function was part of some utility, in this specific case it would remove all results that fit the criterion of being used as an input to some Calc class. I think it could still be quickly fixed to do this. But I would suggest to remove it, to avoid there being more functionality than maintenance. Also, it seems that there was no test for this which is my fault.

Thanks for the quick and detailed reply! I will make a PR, where I will remove it and possibly keep it open for anyone else who wants to remove more from the QB that has no purpose anymore in the code base 😅

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

Successfully merging a pull request may close this issue.

4 participants