Skip to content

remove unused ConnectionManagerBase::reenable* functions #121

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.

Sign up for GitHub

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

Jump to bottom
Merged
mgeplf merged 7 commits into from
May 7, 2025
Merged

remove unused ConnectionManagerBase::reenable* functions #121

mgeplf merged 7 commits into from
May 7, 2025

Conversation

@mgeplf
Copy link
Collaborator

@mgeplf mgeplf commented Mar 3, 2025

No description provided.

@mgeplf mgeplf requested a review from juanjosegarcan April 28, 2025 08:59
@mgeplf mgeplf marked this pull request as ready for review April 28, 2025 08:59
@mgeplf mgeplf requested review from WeinaJi and cattabiani April 28, 2025 08:59
@cattabiani
Copy link
Contributor

cattabiani commented Apr 28, 2025

I feel I dunno enough (I do not know anything. it is the first time I see these snippets) and I cannot comment if we need still need this or not. @WeinaJi ?

@cattabiani
Copy link
Contributor

cattabiani commented Apr 28, 2025

(also, a small description or pointing to an issue would be nice)

@juanjosegarcan
Copy link
Contributor

juanjosegarcan commented Apr 28, 2025

LGTM from code pov but as Catta I can not say much more.

@mgeplf
Copy link
Collaborator Author

mgeplf commented Apr 28, 2025

@cattabiani > (also, a small description or pointing to an issue would be nice)

What would you add to remove unused ConnectionManagerBase::reenable* functions?

@cattabiani
Copy link
Contributor

cattabiani commented Apr 28, 2025

@mgeplf

Example of questions that may be explained

  • what was it use for?
  • why is it no more relevant?
  • why there were no tests about this?

@mgeplf
Copy link
Collaborator Author

mgeplf commented Apr 28, 2025

I can't answer any of those; as far as I can tell, it was unused since it's never referenced by anything else, which is also why it's not relevant and probably why there were no tests for it.

@WeinaJi
Copy link
Collaborator

WeinaJi commented Apr 28, 2025

I can't answer any of those; as far as I can tell, it was unused since it's never referenced by anything else, which is also why it's not relevant and probably why there were no tests for it.

I guess those functions were used by some scientists at the BBP time. There are also functions like disable_group, disable which seems not tested.
Is there an issue to track this change? We could discuss further in the ticket.

@mgeplf
Copy link
Collaborator Author

mgeplf commented Apr 28, 2025

Is there an issue to track this change? We could discuss further in the ticket.

There isn't an issue, I guess I could make one; is there an advantage to that compared to having the discussion here?

@cattabiani
Copy link
Contributor

cattabiani commented Apr 28, 2025

because this discussion will be lost a little no?

@mgeplf
Copy link
Collaborator Author

mgeplf commented Apr 28, 2025

Not sure how it would get lost, but I have made a ticket: #225

mgeplf added 2 commits May 6, 2025 15:45
@mgeplf
Merge remote-tracking branch 'origin/main' into remove-reenable
c919cf9
@mgeplf
Remove ConnectionManagerBase functionality that isn't used:
a671f2f 
    ConnectionManagerBase::delete
    ConnectionManagerBase::disable
    ConnectionManagerBase::delete_group
    ConnectionManagerBase::disable_group
    ConnectionManagerBase::get_disabled
WeinaJi
WeinaJi approved these changes May 7, 2025
View reviewed changes
Copy link
Collaborator

@WeinaJi WeinaJi left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@mgeplf mgeplf merged commit ef3c742 into main May 7, 2025
18 checks passed
@mgeplf mgeplf deleted the remove-reenable branch May 7, 2025 09:01
@mgeplf mgeplf mentioned this pull request May 27, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@WeinaJi WeinaJi WeinaJi approved these changes

@juanjosegarcan juanjosegarcan Awaiting requested review from juanjosegarcan

@cattabiani cattabiani Awaiting requested review from cattabiani

Assignees

@mgeplf mgeplf

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mgeplf @cattabiani @juanjosegarcan @WeinaJi