added builtin list::reverse for YCP list #3

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
3 participants

No description provided.

libycp/src/YCPBuiltinList.cc
+ * @param list<flex1> v list
+ * @param integer i1 index of first element
+ * @param integer i2 index of second element
+ * @return New list. Changed if offset is correct, otherwise return unchanged list
@jreidinger

jreidinger Jun 20, 2012

Owner

Is the documentation right? I don't see such parameters

+ {
+ ycp2error ("Cannot reverse 'nil' list");
+ return YCPNull ();
+ }
@jreidinger

jreidinger Jun 20, 2012

Owner

is it really problem? If so, then why continue with execution?

@jreidinger

jreidinger Jun 20, 2012

Owner

After reading documentation for you method I strongly don't see reason for such error log - Creates new list with reversed order of elements. Return nil if list is nil. so it is documented as common and expected behavior

@thomas-fehr

thomas-fehr Jun 20, 2012

On Wed, Jun 20, Josef Reidinger wrote:

  • \* @id list.reverse
    
  • \* @short Creates new list with reversed order of elements.
    
  • \* @param list<flex1> v list
    
  • \* @return New list. Changed if offset is correct, otherwise return unchanged list 
    
  • *
    
  • \* @description
    
  • \* Creates new list with reversed order of elements. Return nil if list is nil. 
    
  • *
    
  • \* @usage list::reverse ([0,1,2,3]) -> [3,2,1,0]
    
  • \* @usage list::reverse ([]) -> []
    
  • */
    
  • if (v.isNull ())
  • {
  • ycp2error ("Cannot reverse 'nil' list");
  • return YCPNull ();
  • }

After reading documentation for you method I strongly don't see reason for such error log - Creates new list with reversed order of elements. Return nil if list is nil. so it is documented as common and expected behavior

This is first time I add an ycp builtin.

I just copied code of another builtin and changed functionality
so I have no idea if this error log has some reason or not.

Tschuess,

Thomas Fehr

Thomas Fehr, SuSE Linux Products GmbH, Maxfeldstr. 5, 90409 Nuernberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg)
Tel: +49-911-74053-0, Fax: +49-911-74053-482, Email: fehr@suse.de
GPG public key available.

@jreidinger

jreidinger Jun 20, 2012

Owner

It depend case by case like for add new element to nil list it is reasonable to have it here.
From my POV reduce is similar case like e.g. sort ( just return list with different order ), so it make sense for me to not have here error message.
@mvidner Can you bring some light here?

Owner

jreidinger commented Jun 20, 2012

Except two minor issues I think it is fine.
Just one more question, does it show automatatic in generated doc for ycp in ycp buildins?

Thanks for fix. Just please update also return, there is no offset

Owner

thomas-fehr replied Jun 20, 2012

I mean sentence Changed if offset is correct, otherwise return unchanged list It is wrong, you don't pass any offset here, so it is always correct. It make sense only if you have function that have some offset, not here

Owner

thomas-fehr replied Jun 20, 2012

Sorry, misunderstood.
Description of return value is fixed now.

On Wed, Jun 20, Josef Reidinger wrote:

Except two minor issues I think it is fine.
Just one more question, does it show automatatic in generated doc for ycp in ycp buildins?

It is present in YCPBuiltinList.xml in libycp/doc so I would assume it
show up somewhere bit I have no idea where.

Tschuess,

Thomas Fehr

Thomas Fehr, SuSE Linux Products GmbH, Maxfeldstr. 5, 90409 Nuernberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg)
Tel: +49-911-74053-0, Fax: +49-911-74053-482, Email: fehr@suse.de
GPG public key available.

Owner

jreidinger commented Jun 20, 2012

@aschnell I see that you write a most lines here. I think that your review would be also nice. We should keep core in good shape.

@@ -1289,6 +1315,7 @@
{ "reduce", "flex1 (variable <flex1>, variable <flex1>, const list <flex1>, const block <flex1>)", (void *)l_reduce1, DECL_LOOP|DECL_SYMBOL|DECL_FLEX, ETCf },
{ "reduce", "flex1 (variable <flex1>, variable <flex2>, const flex1, const list <flex2>, const block <flex1>)", (void *)l_reduce2, DECL_LOOP|DECL_SYMBOL|DECL_FLEX, ETCf },
{ "swap", "list <flex> (const list <flex>, const integer, const integer)", (void *)l_swaplist, DECL_FLEX, ETCf },
+ { "reverse", "list <flex> (const list <flex>)", (void *)l_reverselist, DECL_FLEX, ETCf },
@jreidinger

jreidinger Jun 20, 2012

Owner

Well, this line is a little magic for me, but from list of DECL_FLEX it looks like it should also accept nil so DECL_NIL
@mvidner can you point me to some documentation to verify it? ( at least it is strange for me why only select have it here, but almost all function handle nil list )

@mvidner

mvidner Jun 26, 2012

Owner

DeclFlags are declared at

I cannot quite parse your question, but anyway, the problem is that we haven't really agreed on whether nil is an "exception" or a processable value.

@jreidinger

jreidinger Jun 26, 2012

Owner

I see that definition of DeclFlags, but I don't understand it completelly especially part with DECL_NIL. It looks bogus for me. Some methods doesn't have it and still it looks like ti accept nil paramter.

mvidner added a commit to mvidner/yast-core that referenced this pull request Jul 11, 2012

Merge pull request #3 from thomas-fehr/master
Conflicts:
	package/yast2-core.changes
Owner

mvidner commented Jul 11, 2012

Obsoleted by pull rq #7.

@mvidner mvidner closed this Jul 11, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment