Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH+RF: inform which unit value is actually overwritten (ignored and set) #1219

Merged

Conversation

yarikoptic
Copy link
Contributor

While at it I RF code to avoid duplication (evil, tiresome to change) in the touched code

Motivation

Otherwise user is left wondering "what was ignored? it might have been millivolts and now it became volts?" and alike.

@yarikoptic yarikoptic force-pushed the enh-whichunit-is-ignored branch from 63ae59b to 262e384 Compare April 10, 2020 00:51
@rly
Copy link
Contributor

rly commented Apr 11, 2020

Thanks @yarikoptic . Looks good. Could you please fix the flake8 errors:

tests/unit/test_icephys.py:252:121: E501 line too long (123 > 120 characters)
tests/unit/test_icephys.py:302:121: E501 line too long (126 > 120 characters)

…set)

Otherwise user is left wondering "what was ignored? it might have been millivolts and now it became volts?" and alike.
While at it I RF code to avoid duplication (evil, tiresome to change) in the touched code
@yarikoptic yarikoptic force-pushed the enh-whichunit-is-ignored branch from 262e384 to 4ae624a Compare April 11, 2020 14:42
@yarikoptic
Copy link
Contributor Author

doh -- thanks for the buzz. force pushed the fixed version.
Someone could look into configuring pre-commit for this to ensure that all commits are flake8 ok.

FWIW in dandi-cli @satra inflicted on us pre-commit with black magic -- so it reformats code upon commit to be consistent etc: https://github.com/dandi/dandi-cli/blob/master/.pre-commit-config.yaml . I kinda like it now ;)

@satra
Copy link

satra commented Apr 11, 2020

an aside:

@rly any reason unit names are plural? btw, we are trying to adopt this: https://people.csail.mit.edu/jaffer/MIXF/MIXF-10 across several projects so units are parseable. i don't know if nwb has any standard for unit specification.

@rly
Copy link
Contributor

rly commented Apr 14, 2020

@yarikoptic the pre-commit hooks look useful! I will try to add them. As far as I can tell, adding the file .pre-commit-config.yaml doesn't change anyone's git setup. The user has to install pre-commit and set up the git hook scripts for the hooks to run. So, it's safe to add the YAML file to the root. The flake8 hook would be great too.

@satra Yarik and I have been discussing the units issue here: NeurodataWithoutBorders/nwb-schema#324 I will raise the issue with the others in the core dev team later this week.

@rly rly merged commit 38d7f51 into NeurodataWithoutBorders:dev Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants