Closed cmarkle closed 9 years ago
We should probably start a v4 tracker thread to mark what needs to be removed theres some other stuff in here that needs moving too like Account#find_vlan_with_number should probably get moved into a NetworkVlan class once thats created in order to fit with the organization of the other objects. And it doesnt really fit with the other find methods that allowed passing of object filters (it could have been a more generic/powerful find_vlan method) and all find methods could be normalized to add a result limit as well so they follow the API usage get object service, filter, limit results, and call service method.
Just so you know where some of this came from. The goal of sl_attr is really just a simple mechanism to expose the underlying attributes easily (so you didn't have to use ['attribute']). The only time I used a different name for the attribute was when I didn't think the low-level attribute's name did a good job of describing what the attribute was all about. Then I did the snake case for the "improved" names. This is all very subjective which is why it is also inconsistent. Adding consistency would be OK with me.
In broader terms, the modeling throughout the framework is incomplete because I wanted it to be "demand driven". My goals when creating what exists of the modeling layer was not to model everything, but to do "enough" to make your lives easier :-). In some cases (like the VLAN case) I made a judgement call between modeling things "correctly" and getting the functionality incorporated. Please do expand the models as needed to simplify your scripts.
I created a v4.0 milestone that you can assign issues to if you would like.
I can push this change today and mark the old ones with a deprecation warning and reference the new ones if we are ok with starting this transition to fix it for the above attributes.
This would allow us to enable the change now and do the removal of the old ones marked as deprecated with the release of v4 (at the expense of looking ugly in the doc but it allows a smoother transition).
Should we do the same for the class name? VLANFirewall = VlanFirewall .
We can also enable this now by renaming the class and add a alias under the class definition for compat IIRC:
class VlanFirewall
end
VLANFirewall = VlanFirewall
My personal preference is to leave Acronyms and abbreviations in all upper case. I prefer VLAN to Vlan, URL to Url etc... But that's probably just because I'm overly pedantic.
The code is there for you guys to use, not for me to write. You may change it if you feel strongly about it.
Excerpt from the Ruby Style Guide this project is following:
- Use CamelCase for classes and modules. (Keep acronyms like HTTP, RFC, XML uppercase.)
So the classes should stay as is but the VLAN_number attrib should still probably be lowercase.
OK so the PR for this causes duplicate getter methods but allows for transition except in the case of ProductConfigurationOption where its going to also cause duplicate data as well because of it being a struct.
Made some comments in the PR. I like the transition idea.
Im adding the xx_at but keeping them closely named (ie last_edited_at) that way its easier to tell which hash key it goes to. The at will help distinguish them from being confused for possible booleans in the case of created/modified.
I think we are good now, all case cleanups and missing rdoc documentation are in and a few bug fixes with masks as well.
All commits that need to have something removed for v4 have been tagged v4-cleanup-required
in the commit message, additionally for tracking purposes this is the deprecation/change list:
Class | Type | Deprecated Name | New Name |
---|---|---|---|
Account | Attribute | companyName | company_name |
Account | Attribute | firstName | first_name |
Account | Attribute | lastName | last_name |
Account | Attribute | postalCode | postal_code |
Account | Attribute | officePhone | office_phone |
Account | Method | find_VLAN_with_number | find_vlan_with_number |
BareMetalServerOrder_Package | Attribute | provision_script_URI | provision_script_uri |
BareMetalServerOrder | Attribute | provision_script_URI | provision_script_uri |
NetworkComponent | Attribute | maxSpeed | max_speed |
NetworkMessageDelivery | Attribute | created | created_at |
NetworkMessageDelivery | Attribute | modified | modified_at |
NetworkStorageCredential | Attribute | created | created_at |
NetworkStorageCredential | Attribute | modified | modified_at |
NetworkStorageGroup | Attribute | created | created_at |
NetworkStorageGroup | Attribute | modified | modified_at |
NetworkStorage | Attribute | created | created_at |
ProductItemCategory | Attribute | categoryCode | category_code |
ProductConfigurationOption | Key | capacityRestrictionMaximum | capacity_restriction_maximum |
ProductConfigurationOption | Key | capacityRestrictionMinimum | capacity_restriction_minimum |
ProductConfigurationOption | Key | capacityRestrictionType | capacity_restriction_type |
ProductConfigurationOption | Key | hourlyRecurringFee | hourly_recurring_fee |
ProductConfigurationOption | Key | laborFee | labor_fee |
ProductConfigurationOption | Key | oneTimeFee | one_time_fee |
ProductConfigurationOption | Key | recurringFee | recurring_fee |
ProductConfigurationOption | Key | requiredCoreCount | required_core_count |
ProductConfigurationOption | Key | setupFee | setup_fee |
Server | Attribute | fullyQualifiedDomainName | fqdn |
SoftwarePassword | Attribute | created | created_at |
SoftwarePassword | Attribute | modified | modified_at |
Ticket | Attribute | last_modified | last_modified_at |
UserCustomerExternalBinding | Attribute | created | created_at |
UserCustomer | Attribute | created | created_at |
UserCustomer | Attribute | modified | modified_at |
VirtualDiskImage | Attribute | created | created_at |
VirtualDiskImage | Attribute | modified | modified_at |
VirtualServerOrder_Package | Attribute | provision_script_URI | provision_script_uri |
VirtualServerOrder | Attribute | provision_script_URI | provision_script_uri |
VirtualServer | Attribute | provisionDate | provisioned_at |
VirtualServer | Attribute | activeTransaction | active_transaction |
VirtualServer | Attribute | blockDevices | block_devices |
VirtualServer | Attribute | lastOperatingSystemReload | last_operating_system_reload |
VLANFirewall | Attribute | VLAN_number | vlan_number |
VLANFirewall | Method | primaryRouter | primary_router |
VLANFirewall | Method | fullyQualifiedDomainName | fqdn |
I also added the missing result_limit
option to find methods where it made sense (in some of the methods the last service call was in a loop and therefore adding a result limit would not give the expect output).
The pull request related to this has been pulled in, but I forgot to add the comment that marks this fixed.
The following attributes, defined with the sl_attr method are in camel case (to presumably match the lower-level naming) but to fit the Ruby idioms and the project style guidelines they should be in snake case (i.e., xxx_yyy_zzz, not zzzYyyZzz). Here are the ones I found:
Also the VLAN_number attributes, although in snake case, probably should have "VLAN" in lower-case ala "vlan_number".
I realize that this in effect changes the API but we should get consistent and do it soon...