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

Rename convert scripts #828

Merged
merged 7 commits into from
Dec 7, 2023
Merged

Conversation

arnaudbore
Copy link
Contributor

@arnaudbore arnaudbore commented Dec 4, 2023

scil_convert_tractogram.py -> scil_tractogram_convert.py
scil_convert_gradients_mrtrix_to_fsl.py -> scil_gradients_convert_mrtrix_to_fsl.py
scil_convert_gradients_fsl_to_mrtrix.py -> scil_gradients_convert_fsl_to_mrtrix.py
scil_convert_rgb.py -> scil_rgb_convert.py

@arnaudbore arnaudbore changed the title [WIP] Rename convert scripts Rename convert scripts Dec 4, 2023
@arnaudbore arnaudbore requested a review from frheault December 4, 2023 21:36
@arnaudbore
Copy link
Contributor Author

Build passed ! Good Job 🍻 !

@arnaudbore
Copy link
Contributor Author

Build passed ! Good Job 🍻 !

Copy link
Contributor

@karanphil karanphil left a comment

Choose a reason for hiding this comment

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

Nothing to say except that I don't think scil_convert_rgb.py should go in the reconst module. Maybe scil_io_convert?

@gabknight
Copy link
Contributor

For scil_convert_rgb.py, I'm not sure for either reconst or io for this one. If we go with io, scil_convert_tractogram should also be io to be consistent, but I prefer it as scil_tractogram_convert. I think reconst, would make the script more easily found when pressing tab.

Nitpicking, I like it more with the verb at the end: scil_reconst_rgb_convert :)

@EmmaRenauld
Copy link
Contributor

EmmaRenauld commented Dec 5, 2023

I think we need to finish PK's PR for reconst first ( #822 ) because if we move the metrics to something else than reconst, then it will be something like scil_metrics_convert_rgb. If we keep them in reconst, then reconst is fine here too.

@arnaudbore
Copy link
Contributor Author

@arnaudbore
Copy link
Contributor Author

@arnaudbore
Copy link
Contributor Author

@arnaudbore
Copy link
Contributor Author

@pep8speaks
Copy link

Hello @arnaudbore, Thank you for updating !

Line 6:80: E501 line too long (80 > 79 characters)

Line 6:80: E501 line too long (80 > 79 characters)

@arnaudbore
Copy link
Contributor Author

Build passed ! Good Job 🍻 !

@arnaudbore arnaudbore merged commit 45ee3ff into scilus:master Dec 7, 2023
5 checks passed
@arnaudbore arnaudbore deleted the rename_convert_scripts branch January 30, 2024 21:57
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.

5 participants