On Tue, Dec 18, 2012 at 8:15 PM, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote: > On Tue, Dec 18, 2012 at 07:57:00PM +0200, Zeeshan Ali (Khattak) wrote: >> On Tue, Dec 18, 2012 at 7:38 PM, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote: >> > On Tue, Dec 18, 2012 at 07:18:04PM +0200, Zeeshan Ali (Khattak) wrote: >> >> On Tue, Dec 18, 2012 at 6:43 PM, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote: >> >> > In my opinion, the second approach is more convenient both for the library >> >> > user and for us from a maintainance point of view, which explains my >> >> > insistance on moving this way ;) >> >> >> >> Thanks for the summary. Thing is that we have already moved a lot >> >> towards the first approach >> > >> > Are you saying the OsinfoInstallScript/OsinfoInstallConfig/ >> > OsinfoInstallConfigParam relationship was intentionally designed this way? >> > Ie OsinfoInstallConfigParam and OsinfoInstallConfig are separate on >> > purpose? >> >> Yes. > > Well, if that's the case and if that was obvious to you, I'm not very > pleased that you mention that now after another patch iteration rather than > in answer to > https://www.redhat.com/archives/virt-tools-list/2012-December/msg00288.html I'm sorry, I just got a bit lost in these changes. >> >Until now my feeling was that this ended up being implemented >> > this way, but that this was not a conscious decision, especially as this >> > code didn't land very long ago. >> >> AFAIK, the code in question has been in a separate branch/list for a >> while and was thoroughly reviewed. > > This was still a huge chunk of code, it's easy to miss some things during > review. I can understand and I didn't mean to blame you. There were some other problematic changes that I also missed during the reviews. >> >> and to be able to move towards the second >> >> approach, we must deprecate the API that are designed for the first >> >> (existing) approach. Otherwise we'll just be confusing app developers >> >> with this contradiction. >> > >> > I'm not exactly sure what API you have in mind exactly. All I'm suggesting >> > is gradual improvement and having a clear idea of what belongs where. I >> > don't think we have things to deprecate to achieve that. >> >> I'm talking about having this config-params on both InstallScript and >> InstallConfig. As you pointed out already, currently app has to look >> at the params on the InstallScript to see which parameters the script >> will be able to use and which ones it must be provided. > > Actually, OsinfoInstallScript does not expose an > OsinfoInstallConfigParamList as this class is introduced by this series. > Not having this class in the first place is one of the reasons I assumed > this part of the code may need some improvements... I did indeed forget that fact that the list itself is not being exposed as is but rather InstallScript only provides methods to access params in the list. > We have various methods on the OsinfoInstallScript class to handle > OsinfoInstallConfigParam, but they look out of place to me, especially > after the introduction of the OsinfoInstallConfigParamList class. I > wouldn't mind deprecating these methods, they are probably not widely used > anyway. None of the libosinfo API is yet widely used. The main app that uses libosinfo is Boxes and we are using the methods in question in there. As I said before, I don't think there is a compelling reason to deprecate APIs in this case. Hence the reason I wanted Daniel to make the decision here. -- Regards, Zeeshan Ali (Khattak) FSF member#5124 _______________________________________________ Libosinfo mailing list Libosinfo@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libosinfo
