Feat/typer and mkdocs #47

mirestrepo · 2020-09-23T20:21:07Z

I'm sorry I disn't separate the documentation from the CLI changes. Once I though about it, it was too late 🤪, so this is probably a hard one to review because there are too many changes.

The cli tests can be run in two different ways , both have pros/cons

Using the type.CLIRunner the cons of this is that the error reporting is not as good. For instance if bad options are passed.... tests fail but quietly
Calling the functions directly. Error reporting is more robust, the annoying thing is that all default arguments must be passed, see running typer cli command from python code fastapi/typer#106

mirestrepo · 2020-09-29T14:41:28Z

This is ready for review. The only puzzle left is that I'd like to make the versions both for the documentation and the package somehow automated with the next release.... but I'm not sure how to do that yet... if you have hints. Let me know

broarr

That was a long PR! I think everything looks good, I just had a couple of little comments. I don't think anything needs to be changed, but I think there are some ways we can better follow UNIX cli conventions

broarr · 2020-09-30T13:19:38Z

docs/bids_postprocess.md

+* `--vv`: Very verbose logging. If True, sets loglevel to DEBUG  [default: False]
+* `--install-completion`: Install completion for the current shell.
+* `--show-completion`: Show completion for the current shell, to copy it or customize the installation.
+* `--help`: Show this message and exit.


Some of these command line options are very non-standard. -ss and --vv in particular. In UNIX like environments -ss is generally equivalent to -s -s and verbose logging is done with -vv (again equivalent to -v -v). I'd suggest -ss gets changed to -S and --vv gets changed to -vv where the number of vs are counted. What do you think?

Of those options the only ones that are defined by us is --vv. The other one are added by typer (I think common to all typer apps). I thought I had -vv initially but it didn't handled for that. I''ll try again, I may not understand how to make that happen in the typer.Option type

broarr · 2020-09-30T13:20:37Z

docs/code_standards.md

+pip install --user commitizen
+```
+
+Then use `cz commit` where you would have previously done `git commit`. -->


Any reason why this is commented out?

I love deleting comments ;-)

I can delete it!

broarr · 2020-09-30T13:21:50Z

docs/code_standards.md

+`-o log_cli=true --log-cli-level=INFO` allows the logger output to go to cli
+`-x` exists on first failure
+
+You will need to have a local `.env` file where you set some environment variables, e.i `XNAT_PASS`


e.i should be i.e. I think

broarr · 2020-09-30T13:24:35Z

docs/code_standards.md

+
+## Documentation
+
+All functions and methods should have an up to date [google style docstring](https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html).  These docstrings are used to build xnat-tools's documentation website.  Additional prose can be added to the website by editing the appropriate markdown file in the `docs/` directory.


I wasn't aware of google style docstrings! Neat!

broarr · 2020-09-30T13:26:47Z

docs/dev_setup.md

+/bin/bash -c "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/master/install.sh)"
+```
+
+[Chocolatey](https://chocolatey.org) is a popular package manager for Windows


To install, run the following from a priviledged powershell prompt:

Set-ExecutionPolicy Bypass -Scope Process -Force; [System.Net.ServicePointManager]::SecurityProtocol = [System.Net.ServicePointManager]::SecurityProtocol -bor 3072; iex ((New-Object System.Net.WebClient).DownloadString('https://chocolatey.org/install.ps1'))

broarr · 2020-09-30T14:04:29Z

xnat_tools/bids_postprocess.py

-        datefmt="%Y-%m-%d %H:%M:%S",
-        handlers=[logging.StreamHandler(sys.stdout)],
+    setup_logging(
+        _logger, log_file, verbose=verbose, very_verbose=very_verbose,


using the count scheme, if verbose is set to 3 we can send that integer into the setup_logging function instead of these two arguments. Then we pick log level based on the integer passed

broarr · 2020-09-30T14:06:05Z

xnat_tools/bids_utils.py

+    elif int(dicomResource["file_count"]) == 0:
+        return False
+
+    return True


It's funny that this stuff is appearing as new

broarr · 2020-09-30T14:07:20Z

xnat_tools/dicom_export.py

+    ),
+    bids_root_dir: str = typer.Argument(
+        ..., help="Root output directory for exporting the files"
+    ),


Is there no way to reuse these arguments via typer? Seems kinda wasteful to have them repeated 3 times throughout xnat-tools

I'll take a look

broarr · 2020-09-30T14:09:40Z

xnat_tools/logging.py

+    if logfile == "":
+        handlers = [logging.StreamHandler(sys.stdout)]
+    else:
+        handlers = [logging.FileHandler(logfile), logging.StreamHandler(sys.stdout)]


Nit: Since logging.StreamHandler(sys.stdout) is always a part of handlers I'd set that up front and optionally add the logfile. Something like:

handlers = [logging.StreamHandler(sys.stdout)] if logfile: handlers.append(logging.FileHandler(logfile))

Just a nitpick though, it's obviously fine this way too :-)

broarr · 2020-09-30T14:10:18Z

xnat_tools/run_heudiconv.py

-    overwrite = args.overwrite
+    Run Heudiconv
+    """
+    print("************************")


Why is this print not a log statement?

You have asked this in few PRs 🤣 .... For heudiconv I didn't figure out a way to intercept their logs correctly, so there is no logging set up in that executable. You started another branch to fix this issue, but that became stale. We'll have to revisit

broarr · 2020-09-30T14:14:13Z

I see what happened with all the new stuff that was mine. I didn't merge my PR and you started this branch off of my PR. I think you can rebase against master now, I just merged my branch. That should clean up this PR a little

…/xnat_tools into feat/add-documentation

mirestrepo · 2020-10-21T22:02:34Z

@broarr I'm done 😅 in case you want to take one more look before merge.

broarr

Smaller PRs, please!

If we're changing the CLI option from seqlist to includeseq I think we should rename that argument everywhere for consistency. I'd still love to see those command-line options for typer loaded once, instead of in 4 different places. I think this code could def be a little DRYer

broarr · 2020-10-22T12:59:29Z

.github/workflows/dev-docs.yml

+          echo "Test push commit to the remote in the workflow" >> test.txt
+
+          git config --global user.name "github-actions[bot]"
+          git config --global user.email "41898282+github-actions[bot]@users.noreply.github.com"


Who is this user?

GitHub Bot!

Did you make the bot?

No, that's their bot.... it's documented in their forums. If you look at the commits from the actions, it shows the octocat as the user

broarr · 2020-10-22T12:59:49Z

.github/workflows/dev-docs.yml

+          typer xnat_tools/xnat2bids.py utils docs --name xnat2bids --output docs/xnat2bids.md
+          typer xnat_tools/dicom_export.py utils docs --name xnat-dicom-export --output docs/dicom_export.md
+          typer xnat_tools/run_heudiconv.py utils docs --name xnat-heudiconv --output docs/run_heudiconv.md
+          typer xnat_tools/bids_postprocess.py utils docs --name bids-postprocess --output docs/bids_postprocess.md


This generates logs for the cli scripts?

it generates markdown docs

broarr · 2020-10-22T13:00:38Z

.github/workflows/dev-docs.yml

+          git config --global user.email "41898282+github-actions[bot]@users.noreply.github.com"
+
+          git add -A
+          git commit -m "docs: Update CLI and CHANGELOG.md"


Fun fact, if you have two -m flags it'll use the first as the title and the second as the body of your commit message!

broarr · 2020-10-22T13:01:52Z

.github/workflows/pre-commit.yml

+    steps:
+    - uses: actions/checkout@v2
+    - uses: actions/setup-python@v2
+    - uses: pre-commit/[email protected]


This links to the .pre-commit.yaml file below?

Maybe we make CCV template repository at a later date?

For precommit templates per language

broarr · 2020-10-22T13:03:22Z

.github/workflows/publish.yml

+          pip install mkdocs mkdocs-material mkdocstrings mkdocs-versioning
+      - name: Set up mkdocs config 
+        run: |
+          export VERSION=$(cat pyproject.toml | grep version | head -1 | awk -F: '{ print $1 }'| sed 's/[\",]//g'| tr -d 'version = ')


Nice one-liner!

broarr · 2020-10-22T13:28:17Z

xnat_tools/__init__.py

@@ -1 +1 @@
-__version__ = "0.1.3"
+__version__ = "0.1.7"


Looks like we're duplicating this value in the pyproject.toml file. Can we load the pyproject.toml version here so there's one source of truth?

Or maybe this is automatically updated?

Commitizen will update them in both places.

There are ways to load the version from the pyproject.toml but then it breaks other things (which I forget which one... maybe Commitizen it self)

That's rad!

broarr · 2020-10-22T13:29:15Z

xnat_tools/bids_postprocess.py

+    ),
+    session_suffix: str = typer.Option(
+        "01",
+        "-ss",


No thoughts here?

broarr · 2020-10-22T13:33:06Z

xnat_tools/dicom_export.py

+    ),
+    session_suffix: str = typer.Option(
+        "01",
+        "-S",


Ahh, I see you've changed -S from -ss here. Looks like there's at least one other script that needs the same change. That's why I was asking about a way to make all commands take the same arguments instead of copying and pasting them into new commands

I totally agree with your point. I didn't see a straight forward way... we could discuss other possibilities

broarr · 2020-10-22T13:35:44Z

xnat_tools/dicom_export.py

    scans = get_scan_ids(connection, host, session)
-    scans = filter_scans(scans, seqlist=seqlist, skiplist=skiplist)
+    scans = filter_scans(scans, seqlist=includeseq, skiplist=skipseq)


Why the name change in includeseq? I personally prefer the consistency especially if we're passing the list around

I think I did it so that the i CLI option was more consistent with the name, and then I was lazy to change the actual variable all around. Consistency is good!

broarr · 2020-10-22T13:36:35Z

xnat_tools/dicom_export.py

@@ -217,19 +147,12 @@ def main(args):
        export_session_dir,
    )

+    # Close connection(I don't think this works)
    connection.delete(f"{host}/data/JSESSION")


This does not work. By calling connection.delete you're actually creating a new session to delete the old session

Okay, I'll remove the line

broarr · 2020-10-22T14:51:24Z

.pre-commit-config.yaml

+      hooks:
+        - id: flake8
+          args:
+          - "--max-line-length=89"


Might not be reading the parameters from the pyproject.toml

…/xnat_tools into feat/add-documentation

broarr · 2020-10-26T18:19:18Z

test.txt

@@ -0,0 +1 @@
+Test push commit to the remote in the workflow


This can be deleted!

Interesting! I deleted the file see 9748d65

So I think I must have something on CI

mirestrepo marked this pull request as draft September 23, 2020 20:21

mirestrepo marked this pull request as ready for review September 29, 2020 14:31

mirestrepo requested a review from broarr September 29, 2020 14:31

broarr approved these changes Sep 30, 2020

View reviewed changes

mirestrepo added 24 commits October 20, 2020 12:16

docs: add scaffolding for mkdocs

add271e

docs: installation

96a8748

docs: minor changes

b9702ce

style: removed unused

08e74e6

style: removed unused, the one line is a flake change

f429a2e

refactor(WIP): start skeleton for typer cli for xnat2bids

cf561e8

refactor(WIP): add dicom_export to xnat2bids_typer

2625339

refactor(dicom_export): clomplete options/inputs and run through

8a2bf22

tests: add missing init files and start testing typer

50e48ec

tests: Complete export-dicom test

171587b

tests(run-heudiconv): activate tests for typer version

2696874

tests: refactor xnat-export and run-heudiconv to use typer

d8817a3

refactor: put dicom-expoert and run-heudiconv back in separate files

5b97076

fix: adjust sintax for commands in separate files

dfc7593

docs: minor updates to readme

bc1cd8d

fix: get xnat2bids and tests back working

2893e74

feat: transition posprocessing to typer

a53d73f

tests: update run_plus to new syntax

b52ed00

tests: install and use typer cli to document excutables

89d7181

docs: code standars

bfc9736

ci: add documenation deployment workflow

3f1884c

fix: bring back lost build-system from pyproject file

67557fe

docs: create .readthedocs.yml

43813ba

docs: try mkdocs-versioning

cda114b

mirestrepo and others added 17 commits October 21, 2020 14:43

Merge branch 'feat/add-documentation' of https://github.com/brown-bnc…

25bd703

…/xnat_tools into feat/add-documentation

docs: Update CLI and CHANGELOG.md

dbd5d6a

docs: changelog

33a9334

docs: Update CLI and CHANGELOG.md

130f8dc

ci: add test workflow

4750f99

docs: Update CLI and CHANGELOG.md

6da9c32

ci(tests): add missing pip installs

e014147

docs: Update CLI and CHANGELOG.md

11f5331

ci: updates to the publish worflow

334738a

ci: don't run dev-docs on push to main branch

527b4f6

Merge branch 'feat/add-documentation' of https://github.com/brown-bnc…

7dff61e

…/xnat_tools into feat/add-documentation

docs: Update CLI and CHANGELOG.md

5cbc14d

ci: build dev-docs only on PR

fec6c5a

Merge branch 'feat/add-documentation' of https://github.com/brown-bnc…

d038d17

…/xnat_tools into feat/add-documentation

ci: on PR checkout HEAD

cdcf7ba

ci(dev-docs): revert to run on push

3c85b01

docs: Update CLI and CHANGELOG.md

bac8754

mirestrepo and others added 2 commits October 21, 2020 19:27

ci(docs): fix the name of the CLI commands

9af9110

docs: Update CLI and CHANGELOG.md

4c20d54

broarr approved these changes Oct 22, 2020

View reviewed changes

docs: typos and hook clarification

be7d4b1

broarr reviewed Oct 22, 2020

View reviewed changes

mirestrepo and others added 3 commits October 22, 2020 11:55

style: consistency for flake8 and session-sufix

9748d65

Merge branch 'feat/add-documentation' of https://github.com/brown-bnc…

b42315f

…/xnat_tools into feat/add-documentation

docs: Update CLI and CHANGELOG.md

f0333e6

mirestrepo merged commit 179989c into master Oct 22, 2020

broarr reviewed Oct 26, 2020

View reviewed changes

mirestrepo mentioned this pull request Oct 27, 2020

Improve documentation #18

Closed

mirestrepo deleted the feat/add-documentation branch November 12, 2020 21:21


		## Documentation

		All functions and methods should have an up to date [google style docstring](https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html). These docstrings are used to build xnat-tools's documentation website. Additional prose can be added to the website by editing the appropriate markdown file in the `docs/` directory.

		@@ -1 +1 @@
		__version__ = "0.1.3"
		__version__ = "0.1.7"

Feat/typer and mkdocs #47

Feat/typer and mkdocs #47

Conversation

mirestrepo commented Sep 23, 2020

mirestrepo commented Sep 29, 2020

broarr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

broarr commented Sep 30, 2020

mirestrepo commented Oct 21, 2020

broarr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment