jolly-good-toolbelt / sphinx_gherkindoc

A tool to convert Gherkin into Sphinx documentation
https://jolly-good-toolbelt.github.io/sphinx_gherkindoc/
11 stars 10 forks source link

Enhancement: add pytest-bdd parser support #29

Closed bradsbrown closed 4 years ago

bradsbrown commented 4 years ago

I think this one is "most of the way" there, though it did reveal some of the weaknesses of having left behind the class structure for file parsing -- it makes it much less clear what needs to differ between behave- and pytest-bdd-parsed files, and adds some (potentially unnecessary) complexity to the work of keeping the two in-sync.

Consider this a "spike verging on mergeability", but if it leads to larger discussions/changes, I'm ok with that as well.

dgou commented 4 years ago

I have the same reaction as Ryan. It feels that separating the parser from the rest of the logic would be a good way to isolate the parser variations from the backend processing (which is mostly formatting). I think of it kinda like how coverage worked, extract into a neutral data structure, then process that data structure. That would also allow for other languages to feed in to the backend by just writing out serialized data (maybe JSON, but whatever). I say this all having only looked at Ryan's comments and not yet having looked at the code. :-)

bradsbrown commented 4 years ago

I’m glad to hear two votes for moving into some cleaner class-based structure. That’s more or less where it felt like this needed to go, once I got to this point. Just wanted to hear that independently confirmed before I put in the work for the next round.

dgou commented 4 years ago

(Edited to add this is with python 3.7.6) (Edited to add poetry --version -> Poetry 0.12.17 ) On master, I spun up a new venv, ran the triumvirate (envsetup, self check, run tests) all golden. Killed that venv:

$ git pr 29 upstream
From github.com:jolly-good-toolbelt/sphinx_gherkindoc
 * [new ref]         refs/pull/29/head -> pr/29
Switched to branch 'pr/29'

spun up a new venv and when running envsetup:

$ ./env_setup.py 
In: /home/dwp/code/sphinx_gherkindoc
Setting up Virtual Environment: /home/dwp/.virtual_envs/8e9d7ef53151dc3

Collecting pip<19
  Using cached pip-18.1-py2.py3-none-any.whl (1.3 MB)
Installing collected packages: pip
  Attempting uninstall: pip
    Found existing installation: pip 20.0.2
    Uninstalling pip-20.0.2:
      Successfully uninstalled pip-20.0.2
Successfully installed pip-18.1
Installing dependencies from lock file

[NonExistentKey]   
'Key "hashes" does not exist.'  

install [--no-dev] [--dry-run] [-E|--extras EXTRAS] [--develop DEVELOP]

Traceback (most recent call last):
  File "./env_setup.py", line 59, in <module>
    main()
  File "./env_setup.py", line 55, in main
    env_setup(args.verbose)
  File "./env_setup.py", line 40, in env_setup
    execute_command_list(__commands_to_run, verbose=verbose)
  File "./env_setup.py", line 31, in execute_command_list
    subprocess.run(shlex.split(command), check=True)
  File "/home/linuxbrew/.linuxbrew/opt/python/lib/python3.7/subprocess.py", line 512, in run
    output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command '['poetry', 'install', '-E', 'pytest-bdd']' returned non-zero exit status 1.
dgou commented 4 years ago

Upgraded poetry to 1.0.3 and the previous error went away. Got this warning:

Installing dependencies from lock file
Warning: The lock file is not up to date with the latest changes in pyproject.toml. You may be getting outdated dependencies. Run update to update them.
dgou commented 4 years ago

Pulled and ran against SNBN repo. Output files in docs and _docs were the same. (default to behave and it did, well, behave itself well)

dgou commented 4 years ago

So I had a few minutes at lunch today, and I tried an experiment. Since I don't have a ready-made pytest-bdd based product, I did the following:

../../.virtual_envs/459314548daf648/lib/python3.7/site-packages/sphinx_gherkindoc/parsers/base.py:12: AttributeError



So it seems, and I am not sure if my experiment is fully valid, that this PR could have two sad consequences, depending on how a user's environment is configured:
1. pytest-bdd as released is installed first, then the sphinx-gherkindoc installed with the pytest-bdd option doesn't operate as exected and the `--parser pytest-bdd` option will break strangely.
2. sphinx-gherkindoc with the pytest-bdd option is installed first, and masquerades as 3.2.1 when it is not quite what it says.

So while one might argue that option 2 is OK, that really only applies if there are no other PRs merged and no other pytest-bdd releases done before Ryan's is.

And, more importantly, all of this working on not depends on the order that a user installs their packages, which is very fragile and would be downright mysterious to anyone who just wants to get some stuff documented.

I think if Ryan's PR had a version dink in it, that _might_ help some, but even then, if pytest-bdd does a new release without that PR, we're back again to having to have a very specific ordering of installation for things to work right.

That leaves me thinking that the best way to move forward with this PR is not to have the the pytest-bdd option install Ryan's PR and be operable without it, but if someone does come along and add in Ryan's PR after the fact, this code will be able to leverage that without requiring any install/uninstall of sphinx-gherkindoc.