search

pyblish / pyblish-base

Pyblish base library - see https://github.com/pyblish/pyblish for details.
Other
127 stars 59 forks source link

clear up confusion around pyblish silent fail #380

Closed hannesdelbeke closed 2 years ago

hannesdelbeke commented 2 years ago

add log for plugin discovery steps change error from debug to error

see https://github.com/pyblish/pyblish-base/issues/374

hannesdelbeke commented 2 years ago

not yet all log.debug are tested, should trigger them all since one of them had a typo in the formatting

mottosso commented 2 years ago

Looks good to me. And yes, at least one test to at least excersize them all would be fab.

hannesdelbeke commented 2 years ago

tested all log.debugs in the maya console and none of them have typos. should be good to merge in.

mottosso commented 2 years ago

You tested it.. manually.. this one time? 😄 Hm, that's quite cheeky, and wouldn't account for anyone making changes to these logs in the future. You sure you're happy with that?

mottosso commented 2 years ago

One of the problem I have with this approach is that, yes, we've put log statements on each Python statement. But are all of them actually called? What are the situations where each of them are called? I bet many if not most of those log statements never happen. Which makes them bloat. But, since they are in there, nobody will ever remove them. Which makes them the worse kind of bloat.

What we should do, is handle the cases you've experienced so far. And explicitly write tests for them, to ensure that that case is given an appropriate message. If you haven't had a look at what the Pyblish tests look like yet, here's a good example.

That's it. A few lines in a function. So long as it's prefixed with test_ it'll automatically get picked up during PRs such as this one, and let anyone know when/if it's no longer true.

hannesdelbeke commented 2 years ago

that sounds like a good approach

Can you give a bit more info on the test setup? ex. there are 3 files named test_plugins. assume some of them are related to legacy code?

PS: All logs have triggered, so none of them are bloat. But your point is still valid.

mottosso commented 2 years ago

The tests are called by a runner called "nose", it's an old-school library that doesn't do much else than call any file prefixed test_*.py and function prefixed with test_. If the function fails, the test fails.

So a new test(s) in any of the existing files should suffice.

hannesdelbeke commented 2 years ago

i've added tests for each possibility

mottosso commented 2 years ago

Thanks @hannesdelbeke, I feel bad for asking this since you've done so much work already.. 😄 But the pre11 folder is for testing plug-ins written for Pyblish 1.1 and prior. Would it be too much to ask that those are just moved out of that folder, and into a new plugins folder inside of the tests/ directory directly? Other than that, this looks great and you've checked all the boxes on my end.

hannesdelbeke commented 2 years ago

yes saw your gitter message and was about to ask if they needed moving 👍

hannesdelbeke commented 2 years ago

all ready for merge :)

mottosso commented 2 years ago

Brilliant, thanks for suffering through my nitpicks! 🥳