Major refactor separating tracing, writing, and formatting #103
Conversation
|
Okay, rebasing to a different branch is a viable workaround, though we lose some context from the previous pull request, but that's not too bad. I hope you remember it's going to take me a while to get to this. |
| ]) | ||
|
|
||
| def format_entry(self, entry, event): | ||
| method = getattr(self, 'format_' + entry.__class__.__name__) |
There was a problem hiding this comment.
Since this is a dynamic function finder perhaps we should catch the AttributeError and handle it somehow?
There was a problem hiding this comment.
It's not that dynamic, it's limited to the contents of that file. The user can't create new entry classes.
There was a problem hiding this comment.
Yeah I was talking more about the contributors not the users. If perhaps handling the exception with a more descriptive error could help future contributions that may require new entry types. But it is also fine having exceptions in development so it's not that major.
|
Sorry, I gave it some thought and decided not to merge this PR. It might provide benefits, but it's taking PySnooper in a different direction than I intended. This adds abstraction that's going to be helpful for building things on top of PySnooper, but honestly, I have little interest in that. I think that anyone sophisticated enough to extend PySnooper is sophisticated enough to configure an actual debugger. |
|
Are you actively trying to stop other people from extending PySnooper, or do you feel that the PR is detrimental in its own right? Even if you don't have interest in extensibility, I think this PR is good for the code in general. |
|
I think you're adding an abstraction, one of an unlimited different abstractions that can be added. The more abstractions you add, the more complex the code becomes to maintain. Therefore I'm careful to only add abstractions that further my goals for the project. Since I don't see how this abstraction does that, I prefer to avoid adding it. |
|
And it goes without saying, but I'll say it anyway: You're always free to fork the code and/or integrate a modified version of it in your bigger project. |
|
I think the code is more complex now. Tracer has no separation of concerns. It has to think about things like truncating files and padding thread names. The tests are full of messy regexes and are a struggle to extend, we're seeing that in action right now. This PR removes about 80 lines and organises everything nicely. The abstractions are not complicated to understand. This project is wildly popular, and I want to contribute to that. I want to do work that will reach many interested users. Trying to compete with this in a separate fork would not be a good idea. |
|
Sorry, I think it's easier for people to see things as simple when they're the ones who thought it up. We both have that bias. I see my version as simpler. If I study yours more deeply I might understand it better and see it as simpler, but I don't want to put in that effort. |
I'm sad that it came to this, and I wish there was a better way, but it's time for me to do that. I've made every contribution to this project that I can think of. I will keep an eye on it, but I don't know how involved I will be. When I release my own version, will you please do me the favour of linking to it near the top of the README? |
|
It's sad you won't be contributing much, but it's not sad that you're forking. You have different goals in mind and it's good that these different goals will be pursued in a place where they're not limited by my goals. I won't link to it near the top of the readme, I'm willing to link to it at the bottom. |
Take 2 of #99 without the history mess I made in git.