Make checkers use object oriented paradigm #656
Conversation
currently, binutils have been implemented. pytest test_checkers to see result.
expat test added
|
@Niraj-Kamdar, @terriko I am not sure whether this is the right time to address #638, since it directly affects my project, maybe I and @Niraj-Kamdar could have worked on it after the completion of our respective projects as a stretch goal. What do you think @terriko ? |
|
@SaurabhK122 don't worry I won't block you. Once, Checkers get converted into classes it will make easy to create more checkers. There is two phases to issue #638. 1) convert every checker into classes and 2) remove binary dependency. I will complete 1st phase by tomorrow and second phase won't be a problem for you. |
|
@SaurabhK122 I'd rather have this done before your project gets underway, since it'll mean a lot less conversion after the fact. If you're already working on stuff, don't let this block you. It's fine to put up PRs for old style checkers until this is merged, and if you have any old ones still in queue we can convert them as we go. |
|
@terriko If we want to avoid blocking and reduce effort of conversion we can create another branch on intel upstream repo and @SaurabhK122 can directly contribute to it and we can merge it ones everything is set to go. |
|
@Niraj-Kamdar Is there much left to do on this? Seems like it is pretty close to done. Creating another branch can cause a lot of overhead. |
|
@pdxjohnny No, there isn't that much left here. I will complete the metaclass and other requested changes in less than two days. |
|
Sounds good. The metaclass should be pretty much drop in ready. |
Co-authored-by: John Andersen <johnandersenpdx@gmail.com>
| @@ -5,7 +5,7 @@ int main() { | |||
| printf("It outputs a few strings normally associated with gnutls-serv 2.1.6"); | |||
| printf("They appear below this line."); | |||
| printf("------------------"); | |||
| printf("gnutls-serv 2.1.6"); | |||
| printf("gnutls-cli 2.1.6"); | |||
There was a problem hiding this comment.
Was this a mistake or does the filename need to be changed to also be gnutls-cli?
There was a problem hiding this comment.
No previous version had it like that.
There was a problem hiding this comment.
Maybe both binary has same version signature.
There was a problem hiding this comment.
I think I'd added this testcase when something wasn't working at one point...
There was a problem hiding this comment.
Hmmm, no wasn't me. Ya I'm of the mind these probably need to be changed back, or we need to be sure of why they're chaging
There was a problem hiding this comment.
That's weird. Let's just fix it now, then? or open an issue to fix it after if you want to get this PR merged ASAP?
| @@ -5,7 +5,7 @@ int main() { | |||
| printf("It outputs a few strings normally associated with gnutls-serv 2.3.11"); | |||
| printf("They appear below this line."); | |||
| printf("------------------"); | |||
| printf("gnutls-serv 2.3.11"); | |||
| printf("gnutls-cli 2.3.11"); | |||
There was a problem hiding this comment.
As above -- contents and filename don't match.
|
BTW, I just want to say how nice it is to see this happening. This makes the checkers so much simpler and easier to read and I think it's going to make adding and maintaining them so much easier going forwards. |
|
Finally docs are also done :) |
|
I have also put FIXME and other warning comments where I thought contains_patterns isn't good even or we require more patterns. |
|
I'll open up an issue for the gnutls-serv vs cli filename stuff so we can sort it out properly later, but the rest of this looks good to go and I don't want to have it hanging beacuse it affects other tests. Merge time! |
No description provided.