New location for scripts #370
Conversation
…ripts` and updated some imports in these two files
Codecov Report
@@ Coverage Diff @@
## develop #370 +/- ##
===========================================
+ Coverage 58.32% 58.85% +0.52%
===========================================
Files 54 54
Lines 4250 4214 -36
===========================================
+ Hits 2479 2480 +1
+ Misses 1771 1734 -37
Continue to review full report at Codecov.
|
|
Done. |
|
The scrips were like that before but were moved to modules instead since it is easier to handle multiple Python installations by starting them using python -m can.logger for example like the documentation that you haven’t updated shows. |
|
What exactly do you mean by different Python installations? Like Python 2.7 and 3 along each other? That shouldn't be a problem since our scripts works on both versions; it shouldn't matter which interpreter it is running in. We could also call it |
|
Scripts are also quite hard to use on windows since it does not use the shebang line. You have to rely on the .py filetype association. Usually you must specify the absolute path to the interpreter and the script to execute the intended script with the interpreter it was installed for. |
|
See #165. In general I don’t see a problem with the current way of exposing applications using modules. It’s more consistent on all platforms. |
|
Hm, I see. Why is Windows so complicated ... Well, we could actually do both, right? I mean, we leave it in the scripts folder so they stay separate for a clean structure. But we could import them from the |
|
How do you import scripts? Wouldn’t it be easier the other way around? We can put the CLI applications in a sub package like can.bin or something, and maybe have scripts that import those if you feel like it is easier for you. |
|
I'd like to continue to support calling via p.s I like the setup.cfg changes for the pytest args etc |
hardbyte
requested review from
christiansandberg
and removed request for
hardbyte
I guess it's quite simple either way around. The point I want to make is that these scripts are something different from the core library and thus should sit somewhere else. I will do it like you proposed, although |
|
@hardbyte @christiansandberg Is this okay? |
|
I think the modules should keep being exposed as before (e.g. can.logger) as it is shorter to type and easier to remember. The script part doesn’t add any value to the user. It’s just good for internal structure. |
Earlier this week, you did seem to not have a problem with that ...
And that's some thing very important in my point of view. Since I personally find it very confusing to have |
|
Sorry, I was a bit unclear. What I mean is that we can remove can/logger.py but in can/init.py we can do ‘from .scripts import logger’. That way the users can still use the shorter python -m can.logger command while we have it separated internally. |
|
I know we’ve discussed this before. I think omitting most interfaces from coverage would be fine since there is little we can unit test. SocketCAN is the only we can execute on Travis. The Kvaser test tries to mock the DLL but there is very little confidence that it will work in a real environment and spending time on a test that doesn’t provide value is mostly a waste of time. |
|
Well, you are right: The interfaces we can test best are virtual and socketcan. But #290 adds some serial can tests. In general, excluding interfaces might lead some to ignore that that is a major point of failure of the library, right?
|
|
Yes it certainly is a major point of failure which is why this library is quite harder to test than other libraries. We have to rely on manual testing unfortunately and we should be able to accept new interface implementations without automatic tests. |
|
Well, we can (and probably have to) still do that. |
|
As long as we accept merging a failed coverage CI pull request due to the lower coverage. IMO it would be best if the CI pass and fail criteria matches what we would accept and deny in a pull request. |
|
Well, in general that won't work, since we do not or rather probably cannot check for sufficient documentation an alike. But if you insist, go ahead and change it. Maybe here: #374 |
| @@ -74,6 +79,7 @@ | |||
| # Code | |||
| version=version, | |||
| packages=find_packages(exclude=["test", "test.*"]), | |||
| scripts=list(filter(isfile, (join("scripts/", f) for f in listdir("scripts/")))), | |||
|
Calling: |
|
Hey Kristian, I am currently short of time, but created #392 so it won't be forgotten. Felix D. |
This PR moved the scripts to a new location. For that, I had to change how pytest is configured to still include the scripts in the coverage data.
Preparation for Lauszus/python_can_viewer#2.