Closed rachmadaniHaryono closed 2 years ago
Hi @rachmadaniHaryono, thanks for the contribution!
I think there's an easier way. We see in your third error that 'error'
in empty in the dict. That's something I didn't plan for, and if we fix the handling of this case, all the next cases will be fixed as well, because the "loading_errors"
, "parsing_errors"
and "objects"
keys are always present. So actually you just need to do this at lines 155 and 156:
- error = result.get("error")
- if error:
+ if "error" in result:
+ error = result["error"]
And we should definitely add a test for it. Could you do that please?
you mean like that?
Exactly! That should cover all cases.
These tests are nice to have (we can keep them), but they don't actually test the fix here.
You can probably patch the collector's process.stdout.readline
method with a lambda that returns a JSON string in which there is "error": "", "traceback": "hello"
and you can then check that the raised CollectionError contains "hello" in its message. Does it make sense :sweat_smile: ? I can give more details if needed :slightly_smiling_face:
i think this cover that error and traceback branching
this will not test the program result is succesful
i will fix error from the quality ci and find out what happen with windows ci test
Ah, I can deal with the Windows one, just fix the quality warnings :)
Windows runs should be fixed if you rebase on main branch.
i fix everything except https://github.com/m-burst/flake8-pytest-style/blob/master/docs/rules/PT012.md
i also tried this
def test():
obj = collector.PythonCollector()
with mock.patch("mkdocstrings.handlers.python.collector.json.loads", return_value=retval):
with pytest.raises(CollectionError) as excinfo:
assert obj.collect("", {})
assert str(excinfo.value) == exp_res
if last asert move to outer part it will raise WPS441 Found control variable used after block: excinfo
if it kept as is it will raise PT012
i choose to ignore PT012 because it make less sense
Oh yes, you did well, PT012 is just annoying (doesn't get with
statements) :slightly_smiling_face:
Argh, it seems I've broken CI again. Let me fix it.
You can rebase on main, sorry :bowing_man:
Oh well, the mkdocs-material dep is missing from the test group. If you don't mind, that's LGTM so I'll merge and fix CI after :slightly_smiling_face:
Thank you very much for your contribution and prompt response to my review!
this is fix for several error i got when trying mkdocs on https://github.com/iamtalhaasghar/yewtube
in the end i still don't know what to change when rebuild_category_lists handle empty dict
legacy error 1
legacy error 2
legacy error 3
legacy error 4