Packit-service: Add pycycle to our pre-commit

[lachmanfrantisek]

When working on some refactor (#426 , #424 ) I found this interesting tool:
https://github.com/bndr/pycycle

Do you think it's worth adding it to our pre-commit config?

• [ ] try it locally, play with the options
• [ ] add the check to the config file
  
  
  
  • [ ] would be nice to update also configs in our other repositories
  
  
• [ ] update contribution guide if needed

[TomasTomecek]

I don't personally see value in this: the stack trace clearly tells you how the circular import happened and then it's up to you to fix it. The tool doesn't seem to help with addressing the issue.

[lachmanfrantisek]

I can see some benefits:

• Early response => you get that info when committing and don't need to run tests.
• Precise error for new-comers / lazy ones. ('Cannot import' can mean multiple things.)
• The tests "should" cover that, but you cannot be 100% sure. (Sounds to me like a halting problem..;)
• Does it cost us something? It's only few lines in the config. Nothing complicated.

[TomasTomecek]

* Does it cost us something? It's only few lines in the config. Nothing complicated.

Yes it does:

1. if it breaks, we need to figure out what to do about it: the more content the more stuff to maintain
2. our tests will run even longer: not sure how long it takes to scan the whole p-s

[TomasTomecek]

* Precise error for new-comers / lazy ones. ('Cannot import' can mean multiple things.)

Okay, this is a clear benefit: it takes some time to parse the stack trace (below).

* Early response => you get that info when committing and don't need to run tests.
* The tests "should" cover that, but you cannot be 100% sure.  (Sounds to me like a halting problem..;)

Tests showed me immediately:

$ pytest...
ImportError while loading conftest '/home/tt/g/packit-service/packit-service/tests/conftest.py'.
tests/conftest.py:36: in <module>
    from packit_service.worker.whitelist import Whitelist
packit_service/worker/whitelist.py:33: in <module>
    from packit_service.service.events import (
packit_service/service/events.py:39: in <module>
    from packit_service.worker.handlers import Handler
packit_service/worker/handlers/__init__.py:31: in <module>
    from packit_service.worker.handlers.abstract import Handler, JobHandler
packit_service/worker/handlers/abstract.py:37: in <module>
    from packit_service.service.events import Event
E   ImportError: cannot import name 'Event' from 'packit_service.service.events' (/home/tt/g/packit-service/packit-service/packit_service/service/events.py)

I'm pretty sure that even basic tests discover circular import immediately. Importing things is the first to happen and the import system touches everything. We have "dynamic" imports only in a few places.

[lachmanfrantisek]

2\. our tests will run even longer: not sure how long it takes to scan the whole p-s

$ time pycycle --here
Project successfully transformed to AST, checking imports for cycles..
No worries, no cycles here!
If you think some cycle was missed, please open an Issue on Github.
Finished.

________________________________________________________
Executed in  311,32 millis    fish           external 
   usr time  260,90 millis    0,00 micros  260,90 millis 
   sys time   29,88 millis  1408,00 micros   28,47 millis 

• That's the benefit of the static analysis...

(The tests job runs much longer than the pre-commit one. We still need to wait for the longest job.)

1. if it breaks, we need to figure out what to do about it: the more content the more stuff to maintain

It can be applied to any check/GitHub app we are using. But for real? Only the PR with the setup.

---

What do others think? Am I the only one who would appreciate that?...;)

[TomasTomecek]

Franto, if you think this would be beneficial for us, feel free to go for it :)

[sakalosj]

i think is not worth it, after some experience is quite easy to recognize and facing first time will force you to dive a little bit into python import system :)
:-1:

[lachmanfrantisek]

i think is not worth it, after some experience is quite easy to recognize and facing first time will force you to dive a little bit into python import system :)

I am more thinking about new people in the project (e.g. GSOC/RHOSC students/interns).

[shreyaspapi]

Hey, Can I work on this?

[lachmanfrantisek]

Hey, Can I work on this?

I am not sure if there is much support for this. If you think it can be useful for you and other GSOC students, we can have it in the pre-commit/CI.

[TomasTomecek]

AFAIK, no one on the team was in favour for this, hence I'd suggest to close this down.

Edit: okay, updating the wording then:

"no one from the team except for the requester, obviously :P

[lachmanfrantisek]

no one on the team

ehm..;)

:+1: vs :-1: :-1: (Others don't care.)

I don't need that so much, but that statement was not so true...

[csomh]

I think something like this would be nice, but I'm concerned enabling this as long as they are "experimental" and the last commit is from 2017.

[lachmanfrantisek]

I think something like this would be nice, but I'm concerned enabling this as long as they are "experimental" and the last commit is from 2017.

Today, I found myself in a situation where the cycle was not recognised. (I cannot reproduce that now, so it might be my fault.)

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.
We are doing this to be sure that the issue is still relevant. Anyone can comment to remove the stale state. (The issues marked with pinned, security, bug or EPIC label
are not considered stale.)