search

Donders-Institute / multiecho

Combine multiple-echoes from a multi-echo BOLD fMRI acquisition.
Apache License 2.0
6 stars 2 forks source link

Use argparse-manpage (fix #18) #19

Closed musicinmybrain closed 1 year ago

musicinmybrain commented 1 year ago

I took a shot at using https://pypi.org/project/argparse-manpage/. This seems to work correctly. The result appears not quite as nice as hand-written, but more than good enough.

The purpose of moving argument parser creation into a different module is to minimize dependencies; anything that needs to be imported to create the man page would have to become part of the build-system requires, so it’s nice to limit that to the standard library.

Quibbles welcome.

marcelzwiers commented 1 year ago

Nice, it looks like a clean solution to me, I'm buying it! I suppose you also want to build BIDScoin manpages in the same way or?

marcelzwiers commented 1 year ago

I've moved the content of your setup.cfg file to the pyproject.toml file (2e1f8e8764bd4f91c01be076669a70a177cd73dc). This requires setuptools >= 62.2.0, is that working for you?

musicinmybrain commented 1 year ago

I've moved the content of your setup.cfg file to the pyproject.toml file (2e1f8e8). This requires setuptools >= 62.2.0, is that working for you?

Yes, that’s cleaner—thanks. I had encountered problems when I tried putting the configuration in pyproject.toml, but the cause must have been a typo or something, because https://github.com/Donders-Institute/multiecho/commit/b4078f0c3505be692279a6c322d687ed47d1f1bf seems to be working just fine.

musicinmybrain commented 1 year ago

Nice, it looks like a clean solution to me, I'm buying it! I suppose you also want to build BIDScoin manpages in the same way or?

Absolutely. I still have some dependencies to deal with before I start actually packaging BIDSCoin for Fedora, but I’ll go ahead and make a PR for the man pages.

Thanks for looking at my issues and PR’s. It’s been really helpful.

marcelzwiers commented 1 year ago

I've moved the content of your setup.cfg file to the pyproject.toml file (2e1f8e8). This requires setuptools >= 62.2.0, is that working for you?

Yes, that’s cleaner—thanks. I had encountered problems when I tried putting the configuration in pyproject.toml, but the cause must have been a typo or something, because b4078f0 seems to be working just fine.

It was not a typo of yours, the example in the README was wrong: https://github.com/praiskup/argparse-manpage/pull/97

musicinmybrain commented 1 year ago

It was not a typo of yours, the example in the README was wrong: praiskup/argparse-manpage#97

That makes perfect sense in retrospect. I’m glad you figured it out.

marcelzwiers commented 1 year ago

With the new argparse-manpage changes, if a general user does a pip install bidscoin he/she gets a manpage folder in the cwd, right? Or is there a way to make the manpage generation optional?

musicinmybrain commented 1 year ago

With the new argparse-manpage changes, if a general user does a pip install bidscoin he/she gets a manpage folder in the cwd, right? Or is there a way to make the manpage generation optional?

At least when using a virtualenv, it gets installed somewhere sensible. Working in a checkout of https://github.com/Donders-Institute/multiecho/commit/b4078f0c3505be692279a6c322d687ed47d1f1bf:

python3 -m venv _e
. _e/bin/activate
pip install build
python -m build
pip install dist/multiecho-0.28-py3-none-any.whl
find . -name '*.1'
./_e/share/man/man1/mecombine.1

The venv is even set up such that man mecombine works, which is better than I expected.

marcelzwiers commented 1 year ago

Absolutely. I still have some dependencies to deal with before I start actually packaging BIDSCoin for Fedora, but I’ll go ahead and make a PR for the man pages.

Perhaps this commit makes your life a bit easier? And it is perhaps also of help to mention that the drmaa dependency is not really important (only for speeding up a few applications), and that the output of the pydeface library (which also requires FSL, which is excluded in NeuroFedora) can also be generated by the users using a separate application. The spec2nii application is of more importance (similar to dcm2niix, which is a non-python dependency, therefore not listed in the toml file, but see BIDScoin's container definitions)

@musicinmybrain I edited this message (which you may not have seen if you reply via email)

musicinmybrain commented 1 year ago

Perhaps this commit makes your life a bit easier? […]

Thank you. That is, in fact, helpful. With the removal of pydeface as a mandatory dependency, I could choose to go ahead and package bidscoin without the extras, and fill in the optional dependencies over time. I’m not sure if that’s what I’ll do, but I could.

musicinmybrain commented 1 year ago

pydeface library (which also requires FSL, which is excluded in NeuroFedora)

Oh, that’s helpful, too. I hadn’t started to look at pydeface yet; now I know I don’t need to. Thanks again.

marcelzwiers commented 1 year ago

I made a start with implementing argeparse-manpage for BIDScoin and realized I need some refactoring (putting shared variables and code in __init__.py) to separate off the non-standard dependencies for the build process. Have you started your PR already? If so, perhaps you should put it on hold until I have finished my refactoring?

musicinmybrain commented 1 year ago

Have you started your PR already?

No, I’ve been distracted by Python 3.12 landing in Fedora Rawhide.

If so, perhaps you should put it on hold until I have finished my refactoring?

Sure! Feel free to ping me when you’d like me to work on it.