Re: [Qgis-developer] [processing] OGR imports full of "None" strings, lately

[Germán Carrillo]

My bad, Victor. I was testing against an old build. Just re-built QGIS and
the None is gone.
Thanks for the fix.

Regards,

Germán

2016-10-19 16:05 GMT-05:00 Victor Olaya :

> hmm, it's strange that you get None...If the value pased is empty or None,
> it should set an empty string as value
>
> see: https://github.com/qgis/QGIS/commit/d911671b0669d3654d85954b52e38a
> 9f5e952d4a
>
> I adapted the test to check that, and it is passing
>
> see: https://github.com/qgis/QGIS/commit/0d09ad1ecd4a850c421bb1820e3d58
> 23dde5d33f
>
> 2016-10-19 22:55 GMT+02:00 Germán Carrillo :
>
>> After rebasing a few hours ago, I'm still having issues (None appearing
>> in ogr2ogr commands) with the same example described in this comment [1]
>> (OGR "Convert format" algorithm).
>>
>> Am I doing something wrong? Should I fix/adjust/add something to OGR
>> algorithms?
>>
>> Regards,
>>
>> Germán
>>
>> --
>> [1] https://github.com/qgis/QGIS/commit/61a10df45283a47782bf
>> 49ac62f9c5e5f9b27b21#commitcomment-19464285
>>
>>
>> 2016-10-18 11:19 GMT-05:00 Victor Olaya :
>>
>>> Yes, they should. I will take care of that
>>>
>>> 2016-10-18 17:48 GMT+02:00 Sandro Santilli :
>>> > On Tue, Oct 18, 2016 at 05:44:43PM +0200, Victor Olaya wrote:
>>> >> I just made this fix, which should solve those issues
>>> >>
>>> >> https://github.com/qgis/QGIS/commit/d7bd5dc50705eec3f37ef82f
>>> c819b5bc47cce0f0
>>> >>
>>> >> Let me know if we need to do something else.
>>> >
>>> > I still think a testcase would be very useful.
>>> > Besides, there seem to be tests checking for "None" (string)
>>> > being returned by setting None (type) in
>>> >
>>> >  python/plugins/processing/tests/ParametersTest.py
>>> >
>>> > Should they be updated ?
>>> >
>>> > --strk;
>>> >
>>>
>>
>>
>>
>> --
>> ---
>>|\__
>> (:>__)(
>>|/
>> Soluciones Geoinformáticas Libres
>> http://geotux.tuxfamily.org/
>> http://twitter.com/GeoTux2
>> http://about.me/germancarrillo
>>
>> 
>>
>
>


-- 
---
   |\__
(:>__)(
   |/
Soluciones Geoinformáticas Libres
http://geotux.tuxfamily.org/
http://twitter.com/GeoTux2
http://about.me/germancarrillo


___
Qgis-developer mailing list
Qgis-developer@lists.osgeo.org
List info: http://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: http://lists.osgeo.org/mailman/listinfo/qgis-developer

Re: [Qgis-developer] [processing] OGR imports full of "None" strings, lately

[Victor Olaya]

hmm, it's strange that you get None...If the value pased is empty or None,
it should set an empty string as value

see:
https://github.com/qgis/QGIS/commit/d911671b0669d3654d85954b52e38a9f5e952d4a

I adapted the test to check that, and it is passing

see:
https://github.com/qgis/QGIS/commit/0d09ad1ecd4a850c421bb1820e3d5823dde5d33f

2016-10-19 22:55 GMT+02:00 Germán Carrillo :

> After rebasing a few hours ago, I'm still having issues (None appearing
> in ogr2ogr commands) with the same example described in this comment [1]
> (OGR "Convert format" algorithm).
>
> Am I doing something wrong? Should I fix/adjust/add something to OGR
> algorithms?
>
> Regards,
>
> Germán
>
> --
> [1] https://github.com/qgis/QGIS/commit/61a10df45283a47782bf49ac62f9c5
> e5f9b27b21#commitcomment-19464285
>
>
> 2016-10-18 11:19 GMT-05:00 Victor Olaya :
>
>> Yes, they should. I will take care of that
>>
>> 2016-10-18 17:48 GMT+02:00 Sandro Santilli :
>> > On Tue, Oct 18, 2016 at 05:44:43PM +0200, Victor Olaya wrote:
>> >> I just made this fix, which should solve those issues
>> >>
>> >> https://github.com/qgis/QGIS/commit/d7bd5dc50705eec3f37ef82f
>> c819b5bc47cce0f0
>> >>
>> >> Let me know if we need to do something else.
>> >
>> > I still think a testcase would be very useful.
>> > Besides, there seem to be tests checking for "None" (string)
>> > being returned by setting None (type) in
>> >
>> >  python/plugins/processing/tests/ParametersTest.py
>> >
>> > Should they be updated ?
>> >
>> > --strk;
>> >
>>
>
>
>
> --
> ---
>|\__
> (:>__)(
>|/
> Soluciones Geoinformáticas Libres
> http://geotux.tuxfamily.org/
> http://twitter.com/GeoTux2
> http://about.me/germancarrillo
>
> 
>
___
Qgis-developer mailing list
Qgis-developer@lists.osgeo.org
List info: http://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: http://lists.osgeo.org/mailman/listinfo/qgis-developer

Re: [Qgis-developer] [processing] OGR imports full of "None" strings, lately

[Germán Carrillo]

After rebasing a few hours ago, I'm still having issues (None appearing in
ogr2ogr commands) with the same example described in this comment [1] (OGR
"Convert format" algorithm).

Am I doing something wrong? Should I fix/adjust/add something to OGR
algorithms?

Regards,

Germán

--
[1]
https://github.com/qgis/QGIS/commit/61a10df45283a47782bf49ac62f9c5e5f9b27b21#commitcomment-19464285


2016-10-18 11:19 GMT-05:00 Victor Olaya :

> Yes, they should. I will take care of that
>
> 2016-10-18 17:48 GMT+02:00 Sandro Santilli :
> > On Tue, Oct 18, 2016 at 05:44:43PM +0200, Victor Olaya wrote:
> >> I just made this fix, which should solve those issues
> >>
> >> https://github.com/qgis/QGIS/commit/d7bd5dc50705eec3f37ef82fc819b5
> bc47cce0f0
> >>
> >> Let me know if we need to do something else.
> >
> > I still think a testcase would be very useful.
> > Besides, there seem to be tests checking for "None" (string)
> > being returned by setting None (type) in
> >
> >  python/plugins/processing/tests/ParametersTest.py
> >
> > Should they be updated ?
> >
> > --strk;
> >
>



-- 
---
   |\__
(:>__)(
   |/
Soluciones Geoinformáticas Libres
http://geotux.tuxfamily.org/
http://twitter.com/GeoTux2
http://about.me/germancarrillo


___
Qgis-developer mailing list
Qgis-developer@lists.osgeo.org
List info: http://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: http://lists.osgeo.org/mailman/listinfo/qgis-developer

Re: [Qgis-developer] [processing] OGR imports full of "None" strings, lately

[Victor Olaya]

Yes, they should. I will take care of that

2016-10-18 17:48 GMT+02:00 Sandro Santilli :
> On Tue, Oct 18, 2016 at 05:44:43PM +0200, Victor Olaya wrote:
>> I just made this fix, which should solve those issues
>>
>> https://github.com/qgis/QGIS/commit/d7bd5dc50705eec3f37ef82fc819b5bc47cce0f0
>>
>> Let me know if we need to do something else.
>
> I still think a testcase would be very useful.
> Besides, there seem to be tests checking for "None" (string)
> being returned by setting None (type) in
>
>  python/plugins/processing/tests/ParametersTest.py
>
> Should they be updated ?
>
> --strk;
>
___
Qgis-developer mailing list
Qgis-developer@lists.osgeo.org
List info: http://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: http://lists.osgeo.org/mailman/listinfo/qgis-developer

Re: [Qgis-developer] [processing] OGR imports full of "None" strings, lately

[Sandro Santilli]

On Tue, Oct 18, 2016 at 05:44:43PM +0200, Victor Olaya wrote:
> I just made this fix, which should solve those issues
> 
> https://github.com/qgis/QGIS/commit/d7bd5dc50705eec3f37ef82fc819b5bc47cce0f0
> 
> Let me know if we need to do something else.

I still think a testcase would be very useful.
Besides, there seem to be tests checking for "None" (string)
being returned by setting None (type) in 

 python/plugins/processing/tests/ParametersTest.py

Should they be updated ?

--strk;

___
Qgis-developer mailing list
Qgis-developer@lists.osgeo.org
List info: http://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: http://lists.osgeo.org/mailman/listinfo/qgis-developer

Re: [Qgis-developer] [processing] OGR imports full of "None" strings, lately

[Victor Olaya]

I just made this fix, which should solve those issues

https://github.com/qgis/QGIS/commit/d7bd5dc50705eec3f37ef82fc819b5bc47cce0f0

Let me know if we need to do something else.

2016-10-18 17:40 GMT+02:00 Sandro Santilli :
> German, I think the best way would be to:
>
>  1) File a ticket, targetted at 2.18 and marked as Sever/Regression
>  2) Make a PR containing an automated testcase showing the problem
>  3) Add a revert of the offending commit in the PR, showing how
> it fixes the testcase.
>
> Do you agree, Victor ?
>
> --strk;
>
> On Tue, Oct 18, 2016 at 10:24:15AM -0500, Germán Carrillo wrote:
>> 2016-10-18 8:42 GMT-05:00 Sandro Santilli :
>>
>> > https://github.com/qgis/qgis/commit/61a10df45283a47782bf49ac62f9c5
>> > e5f9b27b21
>>
>>
>> Hi All,
>>
>> what would be the way to deal with this?
>> It's currently preventing me from pushing my final commit to this PR [1],
>> which I hope can make it into QGIS v.2.18.
>>
>> Regards,
>>
>> Germán
>> --
>> [1] https://github.com/qgis/QGIS/pull/3591
___
Qgis-developer mailing list
Qgis-developer@lists.osgeo.org
List info: http://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: http://lists.osgeo.org/mailman/listinfo/qgis-developer

Re: [Qgis-developer] [processing] OGR imports full of "None" strings, lately

[Sandro Santilli]

German, I think the best way would be to:

 1) File a ticket, targetted at 2.18 and marked as Sever/Regression
 2) Make a PR containing an automated testcase showing the problem
 3) Add a revert of the offending commit in the PR, showing how
it fixes the testcase.

Do you agree, Victor ?

--strk;

On Tue, Oct 18, 2016 at 10:24:15AM -0500, Germán Carrillo wrote:
> 2016-10-18 8:42 GMT-05:00 Sandro Santilli :
> 
> > https://github.com/qgis/qgis/commit/61a10df45283a47782bf49ac62f9c5
> > e5f9b27b21
> 
> 
> Hi All,
> 
> what would be the way to deal with this?
> It's currently preventing me from pushing my final commit to this PR [1],
> which I hope can make it into QGIS v.2.18.
> 
> Regards,
> 
> Germán
> --
> [1] https://github.com/qgis/QGIS/pull/3591
___
Qgis-developer mailing list
Qgis-developer@lists.osgeo.org
List info: http://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: http://lists.osgeo.org/mailman/listinfo/qgis-developer

Re: [Qgis-developer] [processing] OGR imports full of "None" strings, lately

[Germán Carrillo]

2016-10-18 8:42 GMT-05:00 Sandro Santilli :

> https://github.com/qgis/qgis/commit/61a10df45283a47782bf49ac62f9c5
> e5f9b27b21


Hi All,

what would be the way to deal with this?
It's currently preventing me from pushing my final commit to this PR [1],
which I hope can make it into QGIS v.2.18.

Regards,

Germán
--
[1] https://github.com/qgis/QGIS/pull/3591




-- 
---
   |\__
(:>__)(
   |/
Soluciones Geoinformáticas Libres
http://geotux.tuxfamily.org/
http://twitter.com/GeoTux2
http://about.me/germancarrillo


___
Qgis-developer mailing list
Qgis-developer@lists.osgeo.org
List info: http://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: http://lists.osgeo.org/mailman/listinfo/qgis-developer

Re: [Qgis-developer] [processing] OGR imports full of "None" strings, lately

[Sandro Santilli]

On Tue, Oct 18, 2016 at 03:38:27PM +0200, Sandro Santilli wrote:
> On Tue, Oct 18, 2016 at 12:48:18PM +0200, Victor Olaya wrote:
> > >
> > > It sounds like having getParamaterValue() always return a string
> > > (possibly empty) would reduce regression probabilities.
> > >
> > > Or do you think it's important to distinguish between empty string
> > > and None ?
> > 
> > sounds safe to me. And it would be nicer to make that distinction, but
> > i think it is not so important. I cannot think of any major issue with
> > that
> 
> Well, I tried and SURPRISE ! The parameter is actually *set*
> to the "None" string. So I guess the bug is also on the *set* side,
> not (just) *get* one.
> 
> Also, a quick review of the set side (setValue functions) reveal that
> the value of parameters don't necessarely need to be strings ?

It looks like reverting 61a10df45283a47782bf49ac62f9c5e5f9b27b21
removes all those "None" literals.

See
https://github.com/qgis/qgis/commit/61a10df45283a47782bf49ac62f9c5e5f9b27b21


--strk;
___
Qgis-developer mailing list
Qgis-developer@lists.osgeo.org
List info: http://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: http://lists.osgeo.org/mailman/listinfo/qgis-developer

Re: [Qgis-developer] [processing] OGR imports full of "None" strings, lately

[Sandro Santilli]

On Tue, Oct 18, 2016 at 12:48:18PM +0200, Victor Olaya wrote:
> >
> > It sounds like having getParamaterValue() always return a string
> > (possibly empty) would reduce regression probabilities.
> >
> > Or do you think it's important to distinguish between empty string
> > and None ?
> 
> sounds safe to me. And it would be nicer to make that distinction, but
> i think it is not so important. I cannot think of any major issue with
> that

Well, I tried and SURPRISE ! The parameter is actually *set*
to the "None" string. So I guess the bug is also on the *set* side,
not (just) *get* one.

Also, a quick review of the set side (setValue functions) reveal that
the value of parameters don't necessarely need to be strings ?

--strk; 
___
Qgis-developer mailing list
Qgis-developer@lists.osgeo.org
List info: http://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: http://lists.osgeo.org/mailman/listinfo/qgis-developer

Re: [Qgis-developer] [processing] OGR imports full of "None" strings, lately

[Victor Olaya]

>
> It sounds like having getParamaterValue() always return a string
> (possibly empty) would reduce regression probabilities.
>
> Or do you think it's important to distinguish between empty string
> and None ?
>

sounds safe to me. And it would be nicer to make that distinction, but
i think it is not so important. I cannot think of any major issue with
that
___
Qgis-developer mailing list
Qgis-developer@lists.osgeo.org
List info: http://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: http://lists.osgeo.org/mailman/listinfo/qgis-developer

Re: [Qgis-developer] [processing] OGR imports full of "None" strings, lately

[Sandro Santilli]

On Tue, Oct 18, 2016 at 12:45:10PM +0200, Sandro Santilli wrote:
> On Tue, Oct 18, 2016 at 12:37:39PM +0200, Victor Olaya wrote:
> > I made a fix to the check that verifies if a string parameter is left
> > blank or not. Before, it just checked that it was None, so empty
> > strings where considered valid values. However, an empty string should
> > be considered a null one (mainly, to raise an exception if that is
> > used for a parameter that is not optional). Before that, an empty
> > string was accepted even if the parameter was mandatory.
> > 
> > looks like ogr algorithms use unicode(getParameterValue()), and they
> > should instead get the param value and check whether is None or not.
> > 
> > Actually, there is no need to cast it to str or unicode, the value is
> > already a string. So it looks like ogr algorithms have to be modified
> > to adapat to this.
> > 
> > A simpler solution is that now, when a null value is passed (None,
> > empty string, anything that evaluates to false...), it sets the value
> > of the parameter as None. It could set it to an empty string, so no
> > need to do any change. Not such a clean solution, but it will work.
> > 
> > Any thoughts on that?
> 
> It sounds like having getParamaterValue() always return a string
> (possibly empty) would reduce regression probabilities.
> 
> Or do you think it's important to distinguish between empty string
> and None ?

Note that I see more than Ogr algortihms using the unicode(getParam())
construct:

  git grep  'unicode(self.getParameterValue' python/plugins/

As long as unicode(None) returns u'None', all those calls are bound
to fail if getParametervalue can possibly return None.

I agree it would be cleaner to drop the unicode() wrapper while making
sure len(val) is avoided. Not sure about the testcase coverage that
would help with such a big change.

--strk;
___
Qgis-developer mailing list
Qgis-developer@lists.osgeo.org
List info: http://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: http://lists.osgeo.org/mailman/listinfo/qgis-developer

Re: [Qgis-developer] [processing] OGR imports full of "None" strings, lately

[Sandro Santilli]

On Tue, Oct 18, 2016 at 12:37:39PM +0200, Victor Olaya wrote:
> I made a fix to the check that verifies if a string parameter is left
> blank or not. Before, it just checked that it was None, so empty
> strings where considered valid values. However, an empty string should
> be considered a null one (mainly, to raise an exception if that is
> used for a parameter that is not optional). Before that, an empty
> string was accepted even if the parameter was mandatory.
> 
> looks like ogr algorithms use unicode(getParameterValue()), and they
> should instead get the param value and check whether is None or not.
> 
> Actually, there is no need to cast it to str or unicode, the value is
> already a string. So it looks like ogr algorithms have to be modified
> to adapat to this.
> 
> A simpler solution is that now, when a null value is passed (None,
> empty string, anything that evaluates to false...), it sets the value
> of the parameter as None. It could set it to an empty string, so no
> need to do any change. Not such a clean solution, but it will work.
> 
> Any thoughts on that?

It sounds like having getParamaterValue() always return a string
(possibly empty) would reduce regression probabilities.

Or do you think it's important to distinguish between empty string
and None ?

--strk;
___
Qgis-developer mailing list
Qgis-developer@lists.osgeo.org
List info: http://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: http://lists.osgeo.org/mailman/listinfo/qgis-developer

Re: [Qgis-developer] [processing] OGR imports full of "None" strings, lately

[Victor Olaya]

I made a fix to the check that verifies if a string parameter is left
blank or not. Before, it just checked that it was None, so empty
strings where considered valid values. However, an empty string should
be considered a null one (mainly, to raise an exception if that is
used for a parameter that is not optional). Before that, an empty
string was accepted even if the parameter was mandatory.

looks like ogr algorithms use unicode(getParameterValue()), and they
should instead get the param value and check whether is None or not.

Actually, there is no need to cast it to str or unicode, the value is
already a string. So it looks like ogr algorithms have to be modified
to adapat to this.

A simpler solution is that now, when a null value is passed (None,
empty string, anything that evaluates to false...), it sets the value
of the parameter as None. It could set it to an empty string, so no
need to do any change. Not such a clean solution, but it will work.

Any thoughts on that?

2016-10-18 12:29 GMT+02:00 Sandro Santilli :
> I've noticed lots of "None" strings in the ogr2ogr call from master_2
> branch onward and it looks like what you get from unicode(None) in
> python.
>
> The code checks for parameter values's length being > 0 before adding
> params to ogr2ogr call but all the empty param are now real "None"
> strings (due to unicode call).
>
> This doesn't seem happen in 2.16 branch.
>
> Victor: something to mass-clean ?
>
> --strk;
___
Qgis-developer mailing list
Qgis-developer@lists.osgeo.org
List info: http://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: http://lists.osgeo.org/mailman/listinfo/qgis-developer

[Qgis-developer] [processing] OGR imports full of "None" strings, lately

[Sandro Santilli]

I've noticed lots of "None" strings in the ogr2ogr call from master_2
branch onward and it looks like what you get from unicode(None) in
python.

The code checks for parameter values's length being > 0 before adding
params to ogr2ogr call but all the empty param are now real "None"
strings (due to unicode call).

This doesn't seem happen in 2.16 branch.

Victor: something to mass-clean ?

--strk; 
___
Qgis-developer mailing list
Qgis-developer@lists.osgeo.org
List info: http://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: http://lists.osgeo.org/mailman/listinfo/qgis-developer