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

Use argparse-manpage (fix #18) #19

Merged

Conversation

musicinmybrain
Copy link
Contributor

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 marcelzwiers self-assigned this Jul 3, 2023
@marcelzwiers marcelzwiers self-requested a review July 4, 2023 06:48
@marcelzwiers
Copy link
Collaborator

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 marcelzwiers merged commit 389eaf1 into Donders-Institute:master Jul 4, 2023
@marcelzwiers
Copy link
Collaborator

marcelzwiers commented Jul 4, 2023

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?

@musicinmybrain
Copy link
Contributor Author

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.

@musicinmybrain
Copy link
Contributor Author

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
Copy link
Collaborator

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:
praiskup/argparse-manpage#97

@musicinmybrain
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

musicinmybrain commented Jul 4, 2023

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 b4078f0:

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
Copy link
Collaborator

marcelzwiers commented Jul 5, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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.

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.

None yet

2 participants