Re: [Development] fixing name of QNetworkAccessManager::createRequest
On 13.01.2012 14:10, Mathias Hasselmann wrote: Am Freitag, den 13.01.2012, 13:00 + schrieb Giuseppe D'Angelo: On 13 January 2012 12:32, Mathias Hasselmannmath...@openismus.com wrote: what about the slightly more garden-variety approach of deprecating the old one and introducing a new method? Maybe we should check the premise of this discussion first. Does that method really have the wrong name? It is called createRequest and after all it causes creation of a network request. Now this network request is just an internal object, and not exposed to the API user. Instead the watchable reply object is returned in advance. The problem is that it doesn't match the Qt namings for the relevant classes... createRequest takes a QNetworkRequest (which is a description of the request that should be made), starts making the underlying network request as you described, but then also creates and returns a reply (QNetworkReply). Bikeshedding? Surely startRequest() would have been a better name. But really not sure it is worth the hassle of changing it. Still Robins suggestions shows a practical approach. 'create' sounds like 'new', therefore QNetworkReply* r = x-createRequest(..); looks like int* i = new char; which is indeed strange. But to think first about what createRequest really does is a good idea: It not only creates a request, it also sends it, so why not rename it to 'doRequest' or something similar which is different to 'createReply' which is as misleading as 'createRequest'. A new name would stop the compiler and there will be no silent bugs. And replacing a simple function name isn't that hard, so there is no need for a 'migration path' via deprecated functions. Peter ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] fixing name of QNetworkAccessManager::createRequest
On Sun, Jan 15, 2012 at 2:25 PM, Peter Kümmel syntheti...@gmx.net wrote: But to think first about what createRequest really does is a good idea: It not only creates a request, it also sends it, so why not rename it to 'doRequest' or something similar which is different to 'createReply' which is as misleading as 'createRequest'. A new name would stop the compiler and there will be no silent bugs. And replacing a simple function name isn't that hard, so there is no need for a 'migration path' via deprecated functions. There would still be silent errors for people who have reimplemented the createRequest method (it's virtual). I think this was covered earlier in this thread. Cheers Rich. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] fixing name of QNetworkAccessManager::createRequest
On 1/15/12 5:08 PM, ext Richard Moore r...@kde.org wrote: On Sun, Jan 15, 2012 at 2:25 PM, Peter Kümmel syntheti...@gmx.net wrote: But to think first about what createRequest really does is a good idea: It not only creates a request, it also sends it, so why not rename it to 'doRequest' or something similar which is different to 'createReply' which is as misleading as 'createRequest'. A new name would stop the compiler and there will be no silent bugs. And replacing a simple function name isn't that hard, so there is no need for a 'migration path' via deprecated functions. There would still be silent errors for people who have reimplemented the createRequest method (it's virtual). I think this was covered earlier in this thread. Yes, let's close it. All the maintainers and Approvers knowing QtNetwork seem to agree (and I actually do as well) to keep the name. Cheers, Lars ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] fixing name of QNetworkAccessManager::createRequest
Off topic: There would still be silent errors for people who have reimplemented the createRequest method (it's virtual). Technically interesting here is the question how such a situation cloud be managed? Using C++11 'final' would prevent the reimplementation. But using pre C++11, the only idea I have is to define a dummy function with a different return value, only this why the compiler would complain. Peter -- Empfehlen Sie GMX DSL Ihren Freunden und Bekannten und wir belohnen Sie mit bis zu 50,- Euro! https://freundschaftswerbung.gmx.de ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] fixing name of QNetworkAccessManager::createRequest
On Sunday, 15 de January de 2012 20.25.17, Peter Kuemmel wrote: Technically interesting here is the question how such a situation cloud be managed? Using C++11 'final' would prevent the reimplementation. But using pre C++11, the only idea I have is to define a dummy function with a different return value, only this why the compiler would complain. Actually, this *is* a solution. If we change the return value of the virtual createRequest to int and keep the arguments, someone trying to override it with the standard function would get a compiler error: stdin:1:224: error: conflicting return type specified for ‘virtual QNetworkReply* Derived::createRequest(QNetworkAccessManager::Operation, const QNetworkRequest, QIODevice*)’ stdin:1:105: error: overriding ‘virtual int QNetworkAccessManager::createRequest(QNetworkAccessManager::Operation, const QNetworkRequest, QIODevice*)’ We can use a return type like ThisFunctionIsDeprecated_InsteadPleaseOverride_createReply. That said, I don't think it's worth it. Just leave it alone. -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel Open Source Technology Center Intel Sweden AB - Registration Number: 556189-6027 Knarrarnäsgatan 15, 164 40 Kista, Stockholm, Sweden signature.asc Description: This is a digitally signed message part. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] fixing name of QNetworkAccessManager::createRequest
On Friday, 13 de January de 2012 08.14.41, Peter Kuemmel wrote: Well, having method createX which creates Y doesn't sound good to me either. Yes, this is worse than a binary-only bug. I don't know the policy for API changes for Qt 5.0, but when such a thing couldn't be fixed, nothing else is worth braking source code compatibility. I would fix it because it is super simple to create a script which simply replaces all the relevant strings in the source files. It's very easy to update the sources once you know that you have to. The problem is that the script might replace things it shouldn't. createRequest is not an exclusive name of QNetworkAccessManager and it might appear elsewhere. The changes we have accepted are either for the greater good or because they can be easily automated with very little mistake (like #includes). What's more, this is a change very hard to track down because it does not create a compiler error if you forget to change your sources. It's a case of silent bugs creeping in. So, changing the name of this function is: - of little practical benefit - not 100% automatable without a code scanner - dangerous, as it creates silent bugs -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel Open Source Technology Center Intel Sweden AB - Registration Number: 556189-6027 Knarrarnäsgatan 15, 164 40 Kista, Stockholm, Sweden signature.asc Description: This is a digitally signed message part. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] fixing name of QNetworkAccessManager::createRequest
Am Freitag, den 13.01.2012, 14:10 +0200 schrieb Robin Burchell: 2012/1/13 Thiago Macieira thiago.macie...@intel.com: So, changing the name of this function is: - of little practical benefit - not 100% automatable without a code scanner - dangerous, as it creates silent bugs what about the slightly more garden-variety approach of deprecating the old one and introducing a new method? Maybe we should check the premise of this discussion first. Does that method really have the wrong name? It is called createRequest and after all it causes creation of a network request. Now this network request is just an internal object, and not exposed to the API user. Instead the watchable reply object is returned in advance. Not really an issue, and not entirely uncommon in async APIs IMHO. Ciao, Mathias -- Mathias Hasselmann math...@openismus.com http://openismus.com/ ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] fixing name of QNetworkAccessManager::createRequest
On 13 January 2012 12:32, Mathias Hasselmann math...@openismus.com wrote: what about the slightly more garden-variety approach of deprecating the old one and introducing a new method? Maybe we should check the premise of this discussion first. Does that method really have the wrong name? It is called createRequest and after all it causes creation of a network request. Now this network request is just an internal object, and not exposed to the API user. Instead the watchable reply object is returned in advance. The problem is that it doesn't match the Qt namings for the relevant classes... createRequest takes a QNetworkRequest (which is a description of the request that should be made), starts making the underlying network request as you described, but then also creates and returns a reply (QNetworkReply). Bikeshedding? -- Giuseppe D'Angelo ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] fixing name of QNetworkAccessManager::createRequest
On Friday, 13 de January de 2012 13.32.27, Mathias Hasselmann wrote: Am Freitag, den 13.01.2012, 14:10 +0200 schrieb Robin Burchell: 2012/1/13 Thiago Macieira thiago.macie...@intel.com: So, changing the name of this function is: - of little practical benefit - not 100% automatable without a code scanner - dangerous, as it creates silent bugs what about the slightly more garden-variety approach of deprecating the old one and introducing a new method? Maybe we should check the premise of this discussion first. Does that method really have the wrong name? It is called createRequest and after all it causes creation of a network request. Now this network request is just an internal object, and not exposed to the API user. Instead the watchable reply object is returned in advance. Not really an issue, and not entirely uncommon in async APIs IMHO. It was supposed to be named sendRequest, but I changed it. I don't know what my reasons were in 2007. -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel Open Source Technology Center Intel Sweden AB - Registration Number: 556189-6027 Knarrarnäsgatan 15, 164 40 Kista, Stockholm, Sweden signature.asc Description: This is a digitally signed message part. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] fixing name of QNetworkAccessManager::createRequest
Am Freitag, den 13.01.2012, 13:00 + schrieb Giuseppe D'Angelo: On 13 January 2012 12:32, Mathias Hasselmann math...@openismus.com wrote: what about the slightly more garden-variety approach of deprecating the old one and introducing a new method? Maybe we should check the premise of this discussion first. Does that method really have the wrong name? It is called createRequest and after all it causes creation of a network request. Now this network request is just an internal object, and not exposed to the API user. Instead the watchable reply object is returned in advance. The problem is that it doesn't match the Qt namings for the relevant classes... createRequest takes a QNetworkRequest (which is a description of the request that should be made), starts making the underlying network request as you described, but then also creates and returns a reply (QNetworkReply). Bikeshedding? Surely startRequest() would have been a better name. But really not sure it is worth the hassle of changing it. Still Robins suggestions shows a practical approach. Ciao, Mathias -- Mathias Hasselmann math...@openismus.com http://openismus.com/ ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] fixing name of QNetworkAccessManager::createRequest
On 01/13/2012 02:10 PM, ext Mathias Hasselmann wrote: (...) Surely startRequest() would have been a better name. But really not sure it is worth the hassle of changing it. Still Robins suggestions shows a practical approach. I don't really think it is worth changing it, I think for 5.0 so far we only broke source compatibility if the name or signature was plain wrong (8ef86d05f199ebab216da43d5e0a9dc322b657b3 or bf7f17060773803f332e8c729a70f47b94243890), not in cases where it could have been named better... Peter Ciao, Mathias ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] fixing name of QNetworkAccessManager::createRequest
createReply doesn't make sense to me. createRequest is perfectly fine in a sense that we query the server, and it returns a reply out of it. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] fixing name of QNetworkAccessManager::createRequest
On 13.01.12 16:25, Sylvain Pointeau wrote: createReply doesn't make sense to me. createRequest is perfectly fine in a sense that we query the server, and it returns a reply out of it. Well, it does make sense to me :-) The request (QNetworkRequest) is already created. What the function actually does is create the reply (QNetworkReply). But I have to agree with Thiago here, let's just keep it how it is. Changing it would introduce too much trouble. Markus ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] fixing name of QNetworkAccessManager::createRequest
Well, having method createX which creates Y doesn't sound good to me either. Yes, this is worse than a binary-only bug. I don't know the policy for API changes for Qt 5.0, but when such a thing couldn't be fixed, nothing else is worth braking source code compatibility. I would fix it because it is super simple to create a script which simply replaces all the relevant strings in the source files. Peter -- Empfehlen Sie GMX DSL Ihren Freunden und Bekannten und wir belohnen Sie mit bis zu 50,- Euro! https://freundschaftswerbung.gmx.de ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] fixing name of QNetworkAccessManager::createRequest
Giuseppe D'Angelo dangelog at gmail.com writes: Being an API break, do you see any good motivation to justify it? Yes, it can only be done in new major version. When if not now? (I agree that the method name is wrong since it creates the reply, but breaking software just because doesn't sound good to me either) Well, having method createX which creates Y doesn't sound good to me either. Regards, Piotr Dobrogost ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] fixing name of QNetworkAccessManager::createRequest
On Monday, 9 de January de 2012 18.46.01, Piotr Dobrogost wrote: Giuseppe D'Angelo dangelog at gmail.com writes: Being an API break, do you see any good motivation to justify it? Yes, it can only be done in new major version. When if not now? Never. (I agree that the method name is wrong since it creates the reply, but breaking software just because doesn't sound good to me either) Well, having method createX which creates Y doesn't sound good to me either. Indeed, but we can't change it if we want to maintain source compatibility. A source compatibility breakage must be justified and I don't think this one is. -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel Open Source Technology Center Intel Sweden AB - Registration Number: 556189-6027 Knarrarnäsgatan 15, 164 40 Kista, Stockholm, Sweden signature.asc Description: This is a digitally signed message part. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
