[prev in list] [next in list] [prev in thread] [next in thread] 

List:       apparmor-dev
Subject:    Re: [apparmor] [PATCH v2] json support for tools (logprof and genprof)
From:       Goldwyn Rodrigues <rgoldwyn () suse ! de>
Date:       2017-04-05 14:21:55
Message-ID: 020e28a9-7d0b-511e-8044-5ae00f9f8876 () suse ! de
[Download RAW message or body]



On 04/03/2017 05:36 PM, Christian Boltz wrote:
> Hallo zusammen,
> 
> Am Montag, 3. April 2017, 17:37:45 CEST schrieb Goldwyn Rodrigues:
>> This is getting more complicated than can be handled. Do you think
>> converting UI module to an abstract class, and deriving JSONUI and
>> TextUI will be a better option? Though it would need more work, it
>> would simplify a lot of things, be more flexible to changes and would
>> be more robust than the hackish way it is being performed now.
> 
> Come on - ui.py is easy and boring. If you think it is complicated, have 
> a look at aa.py ;-)
> 
> I'm not against changes inside ui.py, but I'd like to keep the 
> switch(es) inside that file so that aa-logprof, aa.py etc. don't need to 
> know much about the UI handling. 
> 
> set_json_mode() is such a minimal solution. If a class-based ui.py would 
> still have a similar interface for its callers, I'm ok with the class-
> based way.

Yes, depending on the options passed in CLI, we set the ui variable to
JSONUI or TextUI instance, and call ui.yesnocancel() or ui.GetFile()
No more if then else's which just add to confusion. Just my 2c on OOPS.

> 
> 
> However...
> 
> Your v2 patch loooks nearly finished (my comments probably look scarier 
> than they are ;-) so it's easier and faster to adjust the things Seth 
> and I noticed than starting from scratch ;-)

Some of them are blunders which need to be fixed. Creating a class will
make it more robusts against these blunders. With the current approach,
it will get more crazy if say we think of a new format of output.

> 
> Also note that ui.py lacks test coverage (only 11% covered, and those 
> 11% are basically the "import" lines and the definition of global 
> variables. I don't need to tell you that this makes changes a bit ;-) 
> more risky.
> 
> Personally, I'd prefer to spend some time on writing tests over 
> rewriting ui.py. Or take some time to rewrite ui.py _and_ write tests 
> for it ;-)


For now, I will  fix this patch and try to get in.. also because of the
amount of time required to implement and test the new thing which is
kinda premium given that this has been delayed way too much  now.

> 
>> Finally, is it a principle to not "reply to all" on the mailing list?
>> While the mail does reach me, The ML filters move the mails to
>> respctive folders.
> 
> We don't have an official rule ;-)  Personally I'm not too keen on getting 
> duplicated mails, so replying only to the mailinglist is enough (except 
> if you don't know if someone is subscribed, of course)
> 
> 
> Regards,
> 
> Christian Boltz
> 
> 
> 

-- 
Goldwyn

-- 
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[prev in list] [next in list] [prev in thread] [next in thread]