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

Annotate and fix 'join' module. Update CanId. #314

Merged
merged 6 commits into from
Mar 10, 2019

Conversation

Funth0mas
Copy link
Contributor

@Funth0mas Funth0mas commented Mar 3, 2019

  • Annotate and fix join module.
  • Add CanId uses ArbitrationId.
  • Add encoding header to some files.

It's the last unannotated file except of formats.

Use and test the classmethod CanId factory. Add encoding header to some files.
@codecov-io
Copy link

codecov-io commented Mar 3, 2019

Codecov Report

Merging #314 into development will decrease coverage by <.01%.
The diff coverage is 20.58%.

Impacted file tree graph

@@              Coverage Diff               @@
##           development    #314      +/-   ##
==============================================
- Coverage         39.7%   39.7%   -0.01%     
==============================================
  Files               35      35              
  Lines             6943    6944       +1     
  Branches          1662    1661       -1     
==============================================
  Hits              2757    2757              
- Misses            3975    3976       +1     
  Partials           211     211
Impacted Files Coverage Δ
src/canmatrix/utils.py 90.9% <ø> (ø) ⬆️
src/canmatrix/join.py 0% <0%> (ø) ⬆️
src/canmatrix/tests/test_canmatrix.py 100% <100%> (ø) ⬆️
src/canmatrix/canmatrix.py 78.16% <83.33%> (ø) ⬆️

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 18b7bcf...54cbee8. Read the comment docs.

src/canmatrix/canmatrix.py Outdated Show resolved Hide resolved

import typing

import canmatrix.canmatrix as cm
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ebroecker, are we still abbreviating?

Regardless, isn't everything from canmatrix.canmatrix imported into canmatrix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to from canmatrix import * killer in __init__.py I got unresolvable dependency loop when refactoring copy module #306 . It was simply not possible to import canmatrix.

We must optimize the __init__, than I'm open to refactor to import canmatrix. With help of the IDE it will be super fast. Mypy has found more serious issues than this one...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me I don't need abbreviations.

any hints how to to the __init__.py right are welcome.
Is it as easy as move the content from canmatrix.py to __init__.py or is there a better/nicer way that we don't need to write import canmatrix.canmatrix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not that skilled, but I already learned, that the problem of from canmatrix.canmatrix import * is, that we don't only import all classes (what we generally want), but also all module-level functions (what we usually don't want) and somehow also all modules, which the canmatrix.canmatrix imports. The python import system get confused and for example don't know, whether import copy should be the built-in copy or canmatrix.copy. And if two modules use from another_modul import Something, they generate dependency loop, which can't be resolved.

I'm not sure but I've impression that the __init__ should rather contain concrete list, what to import, for example from canmatrix.canmatrix import CanMatrix, Ecu, Frame, Signal, ... so that it doesn't publish more, than needed. But again, I'm not sure.

src/canmatrix/join.py Outdated Show resolved Hide resolved
src/canmatrix/canmatrix.py Show resolved Hide resolved
src/canmatrix/join.py Outdated Show resolved Hide resolved
@Funth0mas
Copy link
Contributor Author

Funth0mas commented Mar 8, 2019

I reconsidered the classmethods as indeed useless. Please don't merge yet, I will delete them.

@Funth0mas Funth0mas changed the title Annotate and fix 'join' module. Add factory for CanId. Annotate and fix 'join' module. Update CanId. Mar 10, 2019
@ebroecker ebroecker merged commit d4ba001 into ebroecker:development Mar 10, 2019
@Funth0mas Funth0mas deleted the annotate_fix_join branch March 10, 2019 21:53
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.

4 participants