Provide option to return raw metrics by block type (#192 #194
Conversation
…isitor. This is a first commit to implement RawVisitor on top of ComplexityVisitor. Now I have to remove implementations details related to the ComplexityVisitor.
|
@BlaneG Hi Blane, thanks for the contribution! I was not aware of such addition in Python 3.8, I will check it out. The idea is definitely interesting. It's something I wanted to do from the start, but at the time it wasn't possible in an easy way. |
radon/tests/test_ipynb.py
Outdated
| @@ -28,7 +28,7 @@ | |||
| @pytest.mark.skipif(not SUPPORTS_IPYNB, reason="nbformat not installed") | |||
| def test_harvestor_yields_ipynb(log_mock): | |||
| '''Test that Harvester will try ipynb files when configured''' | |||
| target = os.path.join(DIRNAME, 'data/example.ipynb') | |||
| target = os.path.join(DIRNAME, 'data\\example.ipynb') | |||
There was a problem hiding this comment.
This should not be required on Windows, as forward slashes work fine there, as far as I know. But it does break things on Unix platforms, so it has to be reverted.
There was a problem hiding this comment.
I was getting a pytest error with the forward slash:
log_mock = <MagicMock name='log_result' id='1643343259728'>
@pytest.mark.skipif(not SUPPORTS_IPYNB, reason="nbformat not installed")
def test_harvestor_yields_ipynb(log_mock):
'''Test that Harvester will try ipynb files when configured'''
target = os.path.join(DIRNAME, 'data/example.ipynb')
harvester = Harvester([DIRNAME], BASE_CONFIG_WITH_IPYNB)
filenames = list(harvester._iter_filenames())
assert _is_python_file(target)
assert len(filenames) == 1
> assert target in filenames
E AssertionError: assert 'C:\\Users\\b_gra\\Desktop\\projects\\2020\\radon\\radon\\tests\\data/example.ipynb' in ['C:\\Users\\b_gra\\Desktop\\projects\\2020\\radon\\radon\\tests\\data\\example.ipynb']
There was a problem hiding this comment.
@BlaneG I see. But if we hardcode the separator like that the tests will fail on linux. You can keep the forward-slash and use os.path.normpath.
There was a problem hiding this comment.
os.path.normpath? I would personally use the pathlib API, but that's just me.
radon/tests/test_raw_visitor.py
Outdated
| # else: | ||
| # result = analyze(code) | ||
| # assert result == Module(*expected) | ||
| # assert result.loc == result.blank + result.sloc + result.single_comments + result.multi |
There was a problem hiding this comment.
These tests need to be enabled so that we know which ones don't pass.
There was a problem hiding this comment.
@rubik , I suggest deleting these tests since they are not relevant for raw_visitor. The scope of raw.py and raw_visitor.py are different. For example the first couple of tests in test_raw.ANALYZE_CASES include strings and inline code that is not wrapped in a function which should return zeros when metrics are computed from test_raw_visitor.py, but not from raw.py.
I wanted to reuse the test cases but I wasn't sure what the best approach was so this was just a quick hack to filter out the reuseable ones. It probably makes sense to separate out the reuseable tests so the current indexing of test_raw.ANALYZE_CASES isn't so fragile as implemented in test_raw_visitor.py. In other words, I could separate test_raw into something like RAW_AND_VISITOR_CASES and RAW_CASES.
There was a problem hiding this comment.
@BlaneG I see. It makes sense, you can go ahead like you say.
|
I think the idea is good. Now besides the minor comments I added above, two things are needed mainly:
Then before merging, we'll have to format the code with black and isort (I added the commands to the Makefile). |
|
Hi @BlaneG, in order to have this PR merged, you need to fix the conflicts and ensure all tests are passing. Are you still working on it? |
|
I came across this open PR when I was searching to see if there was any progress on the topic - is there anything I can do to help? |
|
@miketheman Hi Mike. Sure, I can merge the PR once:
|
|
There's a big showstopper for ast.get_source_segment: it doesn't roundtrip comments, which makes raw metrics wrong. I'm investigating alternatives that assure a source code roundtrip, hopefully without having to resort to a CST. |
This is a first attempt at implementing a raw_visitor to provide metrics by block type.
It is lightly tested reusing some of the tests from test_raw.py, not all of which are currently passing. Just looking for some feedback @rubik before adding more tests.
ast.get_source_segement was implemented in Python 3.8 which makes it easy to reproduce a segment of the input code from an ast node.