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

Set default output filename to match base path of input #82

Merged
merged 6 commits into from
Nov 19, 2021

Conversation

kspaceKelvin
Copy link
Contributor

This sets a default output filename to match the input name so that it's easier to use.

@hansenms If you have a cleaner way of implementing, please suggest it.

@hansenms
Copy link
Member

hansenms commented Nov 7, 2021

Please merge in the master branch.

I could not from fast searching find a quick way to set a default value from another value in boost options, so maybe it is not possible. @dchansen do you have experience with this? This is not the end of the world here, but it just feels a bit ugly to set the string empty (and so default value which shows up in the help text is empty). but then when the default value is the default value, we actually overwrite it, so it is not the default. Seems bad.

@kspaceKelvin
Copy link
Contributor Author

Are you asking me to merge into the master branch of my fork? I thought GitHub would have figured out the diff now that the skipSyncData PR has been merged, but I still see it in the diff here. I'm evidently not wise in the ways of the GitHub...

@hansenms
Copy link
Member

hansenms commented Nov 7, 2021

you need to merge master into your branch...or rebase your branch, whichever you prefer. If you rebase, you may have to force push, so I would merge the master.

@hansenms
Copy link
Member

hansenms commented Nov 7, 2021

If we cannot find a way to specify the default in terms of another parameter, I would recommend setting the default to something like <input file without extension>.mrd so that it shows up in the help text and then replace that.

@kspaceKelvin
Copy link
Contributor Author

Ah, ok, I fetched the upstream and we're good. Let's wait for David to comment on if it's possible to directly set the default value as you suggested.

@hansenms
Copy link
Member

Looks good.

@dchansen dchansen merged commit 6d0ab3d into ismrmrd:master Nov 19, 2021
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