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

Fix argparse linewrap for R| strings #2685

Merged
merged 2 commits into from
May 7, 2020
Merged

Conversation

cfhammill
Copy link
Contributor

This prevents lines being broken within words when using R| at the beginning of an argparse help string

Useful for #2658

This prevents lines being broken within words when using
R| at the beginning of an argparse help string
@cfhammill cfhammill changed the title utils.py: Fix argparse linewrap for R| strings Fix argparse linewrap for R| strings May 2, 2020
@jcohenadad
Copy link
Member

i tried de580da with sct_create_mask on a Terminal with 100 col, here is the output (notice the broken word):

  -p <str>              Process to generate mask.
                          <coord,XxY>: Center mask at the X,Y coordinates. (e.g. "coord,20x15")
                          <point,FILE>: Center mask at the X,Y coordinates of the label defined in i
nput volume FILE. (e.g. "point,label.nii.gz")

@cfhammill
Copy link
Contributor Author

hmm that's the output I get before the PR, but it's fixed in de580da for me. Could it be a mac thing?

@jcohenadad
Copy link
Member

jcohenadad commented May 2, 2020

Could it be a mac thing?

Yeah that's what i thought... let me try from within docker
EDIT 2020-05-02 15:48:18: For the records, i am using Terminal Version 2.9.5 on OSX Mojave.

@cfhammill cfhammill mentioned this pull request May 2, 2020
19 tasks
@cfhammill cfhammill added this to the 4.2.3 milestone May 2, 2020
@cfhammill cfhammill added documentation category: readthedocs, sourceforge, or SCT courses enhancement category: improves performance/results of an existing feature labels May 2, 2020
@jcohenadad
Copy link
Member

i confirm this is likely a mac thing... in ubuntu (via docker), using the same terminal size, i get the proper word break:

  -p <str>              Process to generate mask.
                          <coord,XxY>: Center mask at the X,Y coordinates. (e.g. "coord,20x15")
                          <point,FILE>: Center mask at the X,Y coordinates of the label defined in
input volume FILE. (e.g. "point,label.nii.gz")

@jcohenadad
Copy link
Member

but regardless, it would be nice to have the text aligned with "<point,FILE>" (which is the case when we do not use "R|"). Do you think that would be possible without too much pain?

@cfhammill
Copy link
Contributor Author

I can think of a pretty rough solution. Just match any number of tabs and spaces in the previous line and insert it in all following lines. I'll see if it works.

A more heavyweight solution that computes offsets for the
paragraph and appends that offset to following lines. Requires
an additional wrapping step compute the new wrap after the
offset is added.
@cfhammill
Copy link
Contributor Author

cfhammill commented May 2, 2020

I think I have a working solution. Not sure what to do about the mac issue. Do we think it's using _split_lines on linux and _fill_text on mac? It could explain why _fill_text is more developed than _split_lines despite apparently not being used on Ubuntu 18.04

@jcohenadad
Copy link
Member

Do we think it's using _split_lines on linux and _fill_text on mac? It could explain why _fill_text is more developed than _split_lines despite apparently not being used on Ubuntu 18.04

no idea...

@kousu kousu self-assigned this May 5, 2020
@alexfoias
Copy link
Contributor

I tried sct_create_mask with terminal 2.10(433), osx 10.15.4 (19E287). And it seems to work well.

Here are my screenshots:
Screen Shot 2020-05-06 at 10 55 09 AM

Screen Shot 2020-05-06 at 10 55 56 AM

@jcohenadad
Copy link
Member

I just tested the updated 3534859 on OSX 10.14 and I get the same output as @alexfoias here, so it looks like your fix fixed it @cfhammill 🥇

@cfhammill
Copy link
Contributor Author

Great, thanks @alexfoias and @jcohenadad. This is ready for review/merge!

Copy link
Member

@jcohenadad jcohenadad 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 the fix @cfhammill !

@jcohenadad jcohenadad merged commit d878016 into master May 7, 2020
@jcohenadad jcohenadad removed the enhancement category: improves performance/results of an existing feature label May 7, 2020
@joshuacwnewton joshuacwnewton deleted the cfh/fix-argparse-linewrap branch May 2, 2023 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation category: readthedocs, sourceforge, or SCT courses
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants