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

refactor: Replace get_vector_from_csv with split_string #791

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

mlitre
Copy link
Contributor

@mlitre mlitre commented Sep 12, 2024

Removed references to get_vector_from_csv by split_string since this last method is more extensible / reusable.

Describe your changes

Issue ticket number and link

Issue #721

Checklist before requesting a review

Removed references to get_vector_from_csv by split_string since this last method is more extensible / reusable.

Signed-off-by: mlitre <[email protected]>
Copy link
Contributor

@maaikez maaikez left a comment

Choose a reason for hiding this comment

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

Maybe you can also remove the function completely? Or is it still useful?

@mlitre
Copy link
Contributor Author

mlitre commented Sep 16, 2024

Maybe you can also remove the function completely? Or is it still useful?

I did not remove to avoid a breaking change. If I'm not mistaken, I saw that this was a public utility so any lib user would get issues if they were using the old method. I do agree that it would be simpler to just remove the function. If that is preferred I can go ahead and do that.

@maaikez
Copy link
Contributor

maaikez commented Sep 16, 2024

Maybe you can also remove the function completely? Or is it still useful?

I did not remove to avoid a breaking change. If I'm not mistaken, I saw that this was a public utility so any lib user would get issues if they were using the old method. I do agree that it would be simpler to just remove the function. If that is preferred I can go ahead and do that.

Yes I understand what you say.
I'm thinking then we can never remove such functions, and for this particular case, there is a replacement. So I would opt for removing it, otherwise other people will use it later again.

What do you think?

@mlitre
Copy link
Contributor Author

mlitre commented Sep 16, 2024

@maaikez fair enough it seems to me to be a pretty low impact change. I'll go ahead and remove the function.

Method completely removed since we can just use the split_string method instead.

Signed-off-by: mlitre <[email protected]>
@mlitre
Copy link
Contributor Author

mlitre commented Sep 16, 2024

@maaikez I've gone ahead and removed the method. Let me know if there are other things I missed in terms of code or process for merging changes / contributing. Thanks!

@maaikez
Copy link
Contributor

maaikez commented Sep 17, 2024

It looks good. Did you somehow check if things still work (except from the unittests)?

Copy link
Contributor

@maaikez maaikez left a comment

Choose a reason for hiding this comment

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

Thanks for contributing!

@mlitre
Copy link
Contributor Author

mlitre commented Sep 17, 2024

It looks good. Did you somehow check if things still work (except from the unittests)?

I have not done much extra testing to be honest, but if necessary I can run some.

@Pietfried Pietfried merged commit 9fca4cb into EVerest:main Sep 20, 2024
4 checks passed
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.

3 participants