Provide option to return raw metrics by block type (#192

[BlaneG]

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.

[rubik]

@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.

[rubik]

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.

[BlaneG]

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']

[rubik]

@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.

[PedanticHacker]

os.path.normpath? I would personally use the pathlib API, but that's just me.

[rubik]

These tests need to be enabled so that we know which ones don't pass.

[BlaneG]

@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.

[rubik]

@BlaneG I see. It makes sense, you can go ahead like you say.

[rubik]

I think the idea is good. Now besides the minor comments I added above, two things are needed mainly:

1. enable all the tests and ensure everything is passing
2. enable this functionality only on Python versions where it's supported. Currently the tests on Travis are failing for this reason.

Then before merging, we'll have to format the code with black and isort (I added the commands to the Makefile).

[rubik]

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?

[miketheman]

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?

[rubik]

@miketheman Hi Mike. Sure, I can merge the PR once:

• all conflicts with the master branch are solved
• all tests pass

[devdanzin]

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.