Refactor get_builder_dt legacy to be explicit #288

nicain · 2017-11-30T02:04:33Z

Motivation

Resolves #258

Debugging and developing legacy files (for example #275) is difficult because the code paths that legacy/map.py::get_builder_dt.py can take are very complicated. This commit refactors this function, to make its behavior explicit and (relatively) easy.

How to test the behavior?

make test

Checklist

Have you checked our Contributing document?
Have you ensured the PR description clearly describes problem and the solution?
Is your contribution compliant with our coding style ? This can be checked running flake8 from the source directory.
Have you checked to ensure that there aren't other open Pull Requests for the same change?
Have you included the relevant issue number using #XXX notation where XXX is the issue number ?

pep8speaks · 2017-11-30T02:04:34Z

Hello @nicain! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on December 04, 2017 at 20:08 Hours UTC

jcfr · 2017-11-30T02:19:24Z

@nicain you could also run flake8 locally to find out about the error.

When addressed, please consider squashing all the commits together.

nicain · 2017-11-30T02:20:53Z

@jcfr I think I have finally figured out how to do that :) Ill add you as a reviewer too.

nicain · 2017-11-30T02:23:52Z

@jcfr OK, flake is complaining about "./src/pynwb/legacy/map.py:26:5: C901 'TypeMapLegacy.get_builder_dt' is too complex (23)"
I want to tell it, well, you should have seen it before! Any thoughts on the refactor? at least its PEP8 compliant...

BTW running flake8 on my dev machine gives > 6000 problems. Is there any way to only run it on the diff?

jcfr · 2017-11-30T02:52:49Z

@jcfr OK, flake is complaining about "./src/pynwb/legacy/map.py:26:5: C901 'TypeMapLegacy.get_builder_dt' is too complex (23)"

If the complexity is legitimate, you can add a comment like this one:

def get_builder_dt(self, builder):  # noqa: C901

running flake8 on my dev machine gives > 6000 problems

This is strange.

Could you confirm you do something like this:

cd /home/jcfr/Projects/pynwb
workon pyenv-py35
flake8
# => No error report

Could you also check you have the latest version of flake8 ?

jcfr · 2017-11-30T02:54:33Z

src/pynwb/legacy/map.py

@@ -25,54 +25,93 @@ class TypeMapLegacy(TypeMap):

    def get_builder_dt(self, builder):


Add a docstring explaining what the function do. One sentence is good enough

jcfr · 2017-11-30T02:58:09Z

src/pynwb/legacy/map.py

+            attrs = builder.attributes
+            ndt = attrs.get('neurodata_type')
+
+            if ndt == 'Module':


To reduce the complexity, consider adding a function named _get_builder_dt_from_neurodata_type(neurodata_type, attrs) ?

I want to keep the functionality as parallel to src/form/build/map.py::get_builder_dt, so I would prefer to use noqa: C901 unless you really think it is unreable w/o a refactor.

jcfr · 2017-11-30T03:01:27Z

src/pynwb/legacy/map.py

+                    return 'ImageSeries'
+                else:
+                    return ancestry
+            elif ndt == 'Custom':


And may be a function named def get_builder_type_from_custom_neurodata_type or similar name.

That said, it is also nice to have all type handling in one function, and add the noqa exception can be legitimate.

same as above

jcfr · 2017-11-30T03:02:16Z

src/pynwb/legacy/map.py

                    return None
                else:
-                    return 'PlaneSegmentation'
-
+                    raise Exception


Avoid generic Exception. Choose from an existing specialized exception RuntimeError or similar . Make also sure to associate a descriptive message

nicain · 2017-11-30T03:28:23Z

Addressing @jcfr:

[x]: Using flake8 locally before push (failure in my dev env due to my .history dir not being --excluded)
[x]: Using # noqa: C901 to skip complexity check (complexity is legitimate)
[x]: Added docstring
[x]: Exception --> RuntimeError
[x]: Squashed commit history

jcfr · 2017-11-30T03:46:36Z

src/pynwb/legacy/map.py

+        For a given builder, return the neurodata_type.  In this legacy
+        TypeMap, the builder may have out-of-spec neurodata_type; this
+        function coerces this to a 2.0 compatible version.
+        '''


One last nitpick: Prefer """ instead of '''

Or for consistency, we can update all docstring in a later commit

jcfr

Assuming CI returns green, LGTM

nicain · 2017-11-30T18:22:07Z

@dorukozturk @jcfr Need a little help here. Travis is complaining about:

The command "pip install -U scikit-ci-addons" failed and exited with 127 during .

, and appveyor hasen't even started after 16 hours. Is there something I did, or a misconfiguration somewhere?

dorukozturk · 2017-11-30T18:30:27Z

@nicain I can take a look at travis.

I don't know about appveyor though. It looks like lot of things got queued. I canceled some of them, I hope that helps.

BTW definitely not something you did. :)

jcfr · 2017-11-30T21:50:51Z

travis

Probably related to https://blog.travis-ci.com/2017-11-21-xcode8-3-default-image-announce

I submitted a PR on a different project to try something. Let's monitor how it goes. See scikit-build/scikit-ci-addons@ee5143d

codecov-io · 2017-12-01T19:16:01Z

Codecov Report

Merging #288 into dev will decrease coverage by 0.24%.
The diff coverage is 2.12%.

@@            Coverage Diff            @@
##              dev    #288      +/-   ##
=========================================
- Coverage   42.84%   42.6%   -0.25%     
=========================================
  Files          35      35              
  Lines        3622    3638      +16     
  Branches      719     727       +8     
=========================================
- Hits         1552    1550       -2     
- Misses       1931    1948      +17     
- Partials      139     140       +1

Flag	Coverage Δ
#form	`42.6% <2.12%> (-0.25%)`	⬇️
#integration	`42.6% <2.12%> (-0.25%)`	⬇️
#pynwb	`42.6% <2.12%> (-0.25%)`	⬇️

Impacted Files	Coverage Δ
src/pynwb/legacy/map.py	`12.12% <2.12%> (-3.88%)`	⬇️
src/pynwb/form/utils.py	`72.91% <0%> (-0.7%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 216cbb3...bf386b3. Read the comment docs.

dorukozturk · 2017-12-04T13:16:55Z

@nicain Let's merge this. There is a major issue with pyenv on travis right now (see this). I will be watching the travis issue and fix it later.

PEP8 compliance PEP8 compliance Add docstring. flake8 complexity skip. meaningful RuntimeError

nicain · 2017-12-04T20:10:07Z

@jcfr Great. I just rebased, incorporating #292. Assuming all but travis passes, please merge.

nicain · 2017-12-04T20:28:17Z

@dorukozturk I clicked in on the appveyor check, and it reports jobs that have been queued for 5 days and targeting commit f960403, not bf386b3 (the current head to be merged in). I wonder if that frozen state that appveyor helped you fix last week is somehow not getting applied to this PR. Either way, it looks like this check needs to be restarted. Can you take a look?

dorukozturk · 2017-12-04T20:56:11Z

@nicain Appveyor let us do 1 build at a time. Since I merged another branch, it was running the master first. Your PR is currently running see this.

nicain requested a review from neuromusic November 30, 2017 02:04

nicain removed the request for review from neuromusic November 30, 2017 02:16

nicain force-pushed the 258 branch from ebcb9b9 to 698d4e4 Compare November 30, 2017 02:19

nicain requested review from neuromusic and jcfr November 30, 2017 02:21

jcfr reviewed Nov 30, 2017

View reviewed changes

nicain force-pushed the 258 branch from 698d4e4 to 462e8c9 Compare November 30, 2017 03:28

jcfr reviewed Nov 30, 2017

View reviewed changes

jcfr previously approved these changes Nov 30, 2017

View reviewed changes

nicain dismissed jcfr’s stale review via 87366c7 November 30, 2017 04:38

nicain force-pushed the 258 branch from 462e8c9 to 87366c7 Compare November 30, 2017 04:38

nicain removed the request for review from neuromusic November 30, 2017 04:41

nicain mentioned this pull request Dec 1, 2017

Fix missing pip issue on travis #292

Merged

Refactor get_builder_dt legacy to be explicit

bf386b3

PEP8 compliance PEP8 compliance Add docstring. flake8 complexity skip. meaningful RuntimeError

nicain force-pushed the 258 branch from 87366c7 to bf386b3 Compare December 4, 2017 20:07

dorukozturk approved these changes Dec 4, 2017

View reviewed changes

dorukozturk merged commit 6437c10 into NeurodataWithoutBorders:dev Dec 4, 2017

nicain mentioned this pull request Dec 15, 2017

KeyError: 'pupil_size_index' in legacy file #301

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor get_builder_dt legacy to be explicit #288

Refactor get_builder_dt legacy to be explicit #288

nicain commented Nov 30, 2017

pep8speaks commented Nov 30, 2017 •

edited

Loading

jcfr commented Nov 30, 2017

nicain commented Nov 30, 2017

nicain commented Nov 30, 2017 •

edited

Loading

jcfr commented Nov 30, 2017

jcfr Nov 30, 2017 •

edited

Loading

jcfr Nov 30, 2017

nicain Nov 30, 2017

jcfr Nov 30, 2017

nicain Nov 30, 2017

jcfr Nov 30, 2017

nicain commented Nov 30, 2017

jcfr Nov 30, 2017

jcfr Nov 30, 2017

jcfr left a comment

nicain commented Nov 30, 2017

dorukozturk commented Nov 30, 2017

jcfr commented Nov 30, 2017

codecov-io commented Dec 1, 2017 •

edited

Loading

dorukozturk commented Dec 4, 2017

nicain commented Dec 4, 2017

nicain commented Dec 4, 2017

dorukozturk commented Dec 4, 2017

		@@ -25,54 +25,93 @@ class TypeMapLegacy(TypeMap):

		def get_builder_dt(self, builder):

Refactor get_builder_dt legacy to be explicit #288

Refactor get_builder_dt legacy to be explicit #288

Conversation

nicain commented Nov 30, 2017

Motivation

How to test the behavior?

Checklist

pep8speaks commented Nov 30, 2017 • edited Loading

Comment last updated on December 04, 2017 at 20:08 Hours UTC

jcfr commented Nov 30, 2017

nicain commented Nov 30, 2017

nicain commented Nov 30, 2017 • edited Loading

jcfr commented Nov 30, 2017

jcfr Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

jcfr Nov 30, 2017

Choose a reason for hiding this comment

nicain Nov 30, 2017

Choose a reason for hiding this comment

jcfr Nov 30, 2017

Choose a reason for hiding this comment

nicain Nov 30, 2017

Choose a reason for hiding this comment

jcfr Nov 30, 2017

Choose a reason for hiding this comment

nicain commented Nov 30, 2017

jcfr Nov 30, 2017

Choose a reason for hiding this comment

jcfr Nov 30, 2017

Choose a reason for hiding this comment

jcfr left a comment

Choose a reason for hiding this comment

nicain commented Nov 30, 2017

dorukozturk commented Nov 30, 2017

jcfr commented Nov 30, 2017

codecov-io commented Dec 1, 2017 • edited Loading

Codecov Report

dorukozturk commented Dec 4, 2017

nicain commented Dec 4, 2017

nicain commented Dec 4, 2017

dorukozturk commented Dec 4, 2017

pep8speaks commented Nov 30, 2017 •

edited

Loading

nicain commented Nov 30, 2017 •

edited

Loading

jcfr Nov 30, 2017 •

edited

Loading

codecov-io commented Dec 1, 2017 •

edited

Loading