-
Notifications
You must be signed in to change notification settings - Fork 575
pebmasquerade plugin #1825
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
pebmasquerade plugin #1825
Conversation
added _UNICODE_STRING length checks.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A number of little issues around this one, it feels like a lot of string manipulation that don't necessarily feel robust. I'll give it another look once you've addressed the comments though...
), | ||
] | ||
|
||
@staticmethod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use classmethods rather than staticmethods. Staticmethods can't determine what class they're a part of, therefore can't access any version information about themselves, which can be important (for instance, for deprecating it in the future).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still needs resolving. Staticmethod
splits the method from the plugin, meaning we can't then use introspection effectively to determine things like which version of the plugin this was attached to. We've run into this before, I realize it feels like it should make no difference, but please use classmethods.
Removed Notes column & path extracted from cmdline and added 2 boolean columns for the length checks.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much and sorry for the delay in getting this re-reviewed. It's looking really good now, just that one outstanding comment that unfortunately does still need resolving, but then I think this can go in! 5:)
), | ||
] | ||
|
||
@staticmethod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still needs resolving. Staticmethod
splits the method from the plugin, meaning we can't then use introspection effectively to determine things like which version of the plugin this was attached to. We've run into this before, I realize it feels like it should make no difference, but please use classmethods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good. I'm a little concerned about false positives because we do our own unicode length maths, and I worry about unusual encodings or something else that throws the count off, but if you're happy this is a useful addition then I have no real problem with it going in. Just need to check that the required_framework_version is actually accurate, and not just copied by default from other plugins... It may well be fine, but if you aren't sure, then I'll like people to get in the habit of setting it to the version they're adding it to please...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks ready for primetime then. 5:)
Hello, trying my way around os internals & memory :P