rdoc for SoftLayer::ImageTemplate class documents two attributes (globalIdentifier and note) that aren't supported about softlayer-ruby

[cmarkle]

I think this is a more broad issue than just in the SoftLayer::ImageTemplate class. Basically anywhere where the sl_attr method is used with the second parameter ala:

sl_attr :global_id, 'globalIdentifier'

Then in addition to rdoc'g the first parameter (e.g., "global_id" in the above) as an attribute, it will rdoc the second as well as an attribute.

[cmarkle]

This kind of thing (from for example the SoftLayer::ImageTemplate class:

##
# :attr_reader:
# The notes, if any, that are attached to the template. Can be nil.
sl_attr :notes, "note"

causes rdoc to treat everything after "sl_attr" as an attribute. I think this will address this:

##
# :attr_reader: notes
# The notes, if any, that are attached to the template. Can be nil.
sl_attr :notes, "note"

But if it's somehow the intent that both "note" and "notes" are actually intended to be attributes then this is not the fix.

Can anyone comment on the intent here? I will submit a patch to fix up the rdoc defs here in the meantime, hoping that that's the way we want to go here...

[ju2wheels]

Right its only the first that should be the attribute. Ive been testing and we need to do this for all the objects for all sl_attr and sl_dynamic_attr entries.

[ju2wheels]

I think it should be this:

##
# :attr_reader: attr_label
sl_attr :attr_label, 'alt_attr_label'

##
# :method: dymanic_attr_label
# :call-seq:
#   dynamic_attr_label(force_update=false)
#
# Description
sl_dynamic_attr :dynamic_attr_label do |param|
   #body
end

[Edit] All sl_dynamic_attr do seem to support the force_update parameter, so these should be marked as :method: and not :attr_reader: and if it doesnt have a :call-seq: section then it needs that as well.

[ju2wheels]

So on the sl_dynamic_attr given that these are actually attributes, I guess it depends on taste how we want to really doc this, if we want to leave it as :attr_reader: it doesnt seem to support :call-seq: but we can indent a block above the description as follows:

##
# :attr_reader: dymanic_attr_label
#   dynamic_attr_label(force_update=false)
#
# Description
sl_dynamic_attr :dynamic_attr_label do |param|
   #body
end

The two space indent causes the method to appear in a "code" block as it does in normal markdown.

[Edit] Atlernative two:

##
# :attr_reader: dynamic_attr_label(force_update=false)
#
# Description
sl_dynamic_attr :dynamic_attr_label do |param|
   #body
end

Would have to drop any :call-seq: sections though.

[cmarkle]

@ju2wheels - You wrote:

[...] we need to do this for all the objects for all sl_attr [...] entries.

For sure we should do this for any sl_attr that has the 2nd argument... Do you think we should specify the rdoc :attr_reader: modifier even for those that don't specify the 2nd argument, i.e., like this case?

##
# :attr_reader:
sl_attr :attr_label

[ju2wheels]

I dont see a need to unless theres a case where its showing up different in the doc like the two arg one.

[cmarkle]

Also I notice that Datacenter.rb seems to have undocumented attributes. Is this on purpose or an oversight?

In Datacenter.rb:

sl_attr :name
sl_attr :long_name, "longName"

[ju2wheels]

Oversight, if its there it should be doc'ed as usable.

[cmarkle]

@ju2wheels - I think I like the option where you specify the calling sequence directly on the :attr_reader: directive, what you called alternative 2, ala:

##
# :attr_reader: dynamic_attr_label(force_update=false)
#
# Description
sl_dynamic_attr :dynamic_attr_label do |param|
    # body
end

That comes out looking like this which I prefer:

Would you be ok with going that way?

Does every sl_dynamic_attr defined attribute support the force_update=false argument? I think from looking at DynamicAttribute.rb that they do. I only see it documented in some of the dynamic attribute cases though...

[ju2wheels]

Yes all of them should support it, I tested it on a dynamic attribute that didnt have it doc'ed and it worked. Its hard to tell from the way the metaprogramming code was written but the getter it defines seems to consistently support that option.

Ill defer to Scott on the doc part for dynamic attrib, I could make the argument for either but given the implementation alt 2 makes the most sense.

[cmarkle]

@SLsthompson - Could I trouble you to weigh in here on a couple of things about how we want to document the attributes created via sl_dynamic_attr?

1. How to rdoc sl_dynamic_attr as attributes? I guess these are unusual in that they are attributes but they take this force_update=true|false optional argument. @ju2wheels made some suggestions as to how to handle this above. I favor just rdoc'g them with the :attr_reader: modifier specifying the full method call ala:

   ##
   # :attr_reader: dynamic_attr_label(force_update=false)
   #
   # Description
   sl_dynamic_attr :dynamic_attr_label do |param|
       # body
   end
   

   This would show the rather unusual "dynamic_attr_label(force_update=false)" in the rdoc attributes section when almost always Ruby attributes would just show as "dynamic_attr_label".

   But another way to go might be to just use the attribute name and somehow make reference to the notion of force_update in the description for the attribute ala:

     ##
     # :attr_reader: dynamic_attr_label
     #
     # Description would somehow mention that there's a force_update=true|false 
     # argument.
     sl_dynamic_attr :dynamic_attr_label do |param|
         # body
     end
   

   A potential benefit here is that the attribute's rdoc output might look more "normal".

2. Include force_update on all such sl_dynamic_attr attributes? Although as @ju2wheels notes, the force_update argument works on all sl_dynamic_attr attributes, it is only mentioned in the description for the ones where there is a 5 minute auto-refresh implemented. We are thinking that we should mention force_update for all sl_dynamic_attr attributes. What do you think?

I'd like, if nothing else for completeness sake, to deal with this now so I'm hoping you can give me some thoughts here.

[SLsthompson]

Hmm... I thought I had added a comment but I don't see it now.

Please take my comments as a member of the community, not as a SoftLayer directive (or edict or whatever...)
1.

If we are to document these as attributes (i.e. with :attr_reader) then I prefer the "oddness" of the top option to overburdening the description for ALL of them.

However, I find myself second guessing using "attr" in the name to begin with. I mean what these really are are "getter" methods. The advantage of documenting them with :attr_reader is that they show up as attributes in rdoc, but they're not REALLY Ruby attributes. I'm kinda back and forth about whether documenting them as such is a good thing.
1.

If you're going to expose the force update parameter for the getter then it should probably be done everywhere.

The other way you might handle this is by finding some way to flag the "getter" routines as being sl_dynamic_attr in the documentation then finding some where in the framework documentation to explain exactly what that means.

[cmarkle]

@SLsthompson:

1. How to doc the dynamic sl_dynamic_attr "attributes"?

   However, I find myself second guessing using "attr" in the name to begin with. I mean what these really are are "getter" methods. The advantage of documenting them with :attr_reader is that they show up as attributes in rdoc, but they're not REALLY Ruby attributes. I'm kinda back and forth about whether documenting them as such is a good thing.

   I consulted with one of our more senior Ruby devs and his inclination was to doc the sl_dynamic_attr things as methods. I am coming around to this position as well. And as methods it's not odd or confusing at all to have the force_update argument. And most of these occasions are documented that way right now, modulo a few which I will fix.

   I think we should leave the sl_attr items as they are now, annotated as attributes via the :attr_reader: specifier.

   So can we agree to:
   a. Document sl_dynamic_attr items as methods (as they mostly are now); and
   b. Document sl_attr items as attributes (:attr_reader:) (as they also mostly are now)

   Sound OK?

2. force_update documented on all sl_dynamic_attr items?

   If you're going to expose the force update parameter for the getter then it should probably be done everywhere.

   For sure I think the force_update parameter should be documented on the methods where there is the "auto update every 5 minutes" scheme, but I think it might already be in those cases. I will verify that.

   @ju2wheels said pretty much the same thing that we should document the force_update parameter everywhere so let me make an update accordingly.

[ju2wheels]

For sure I think the force_update parameter should be documented on the methods where there is the "auto update every 5 minutes" scheme, but I think it might already be in those cases. I will verify that.

I dont think thats true for all of them. Its only in Account that there are dynamic attr entries that decide to update every 5 min if you dont specify force_update (why that was done I have no idea, but it seems to be one of the few oddball updates).

I think default behavior is update if force_update is true or should_update? is true.

[cmarkle]

I think my pull request #98 is ready to go for this.

[SLsthompson]

OK. I will give the changes a quick look. With the expanded Object Filter capability I'll start thinking about a 3.1 release. Is there anything else you guys would like to get into that?

[ju2wheels]

Im going to have the cleanup stuff pushed today (need to add changes based on comments and the result limit options) and the base class for the first half of basic Network Monitors (making some last tweaks to implementation). Thats all I have right now thats ready to go.