search

INPStarfall / Starfall

Starfall Processor for Garry's Mod + Wiremod
http://www.wiremod.com/forum/developers-showcase/22739-starfall-processor.html
Other
17 stars 15 forks source link

Added setSubMaterial and getSubMaterial #393

Closed suunr closed 9 years ago

suunr commented 9 years ago

Implements https://github.com/INPStarfall/Starfall/issues/341

davidsonbr commented 9 years ago

I do like the fact that you state the type, although the first comma , number, description should not be there as it would look odd in the documentation.

Otherwise code style and content are A OK.

awilliamson commented 9 years ago

Hmm, seeing the multiple returns of ent methods makes me question that method. Should we be returning ok, errString? Does it make sense? Probably not.

As @EpicG mentioned, the param name should follow --@param theVar description As the args to the param comment as space delimited.

Interesting use of the 'builder' pattern ( I think it's builder ) 'Chained invocation' paradigm in setMaterial. Returning self allows for chains. Eg `ent:setColor( blah ):setSubMaterial( blah ):setPos( blah ). It's something I've toyed with for a long time now, as it can be handy, but also messy. I think for now, returning the wrapped entity should be removed. I don't think we follow that idea for other methods, so it should remain consistent. If in future we add it, returning self would be sufficient, no need to re-wrap :P

awilliamson commented 9 years ago

@suunrider Those tweaks and changes should be a commit amend to your initial commit for this PR. They most definitely should NOT be separate commits. These don't represent fundamental logical changes, the tweaks suggested are categorised under the same as your original commit.

Please remove the additional 2 commits, look into squashing ( I believe? ). Or simply remove the last 2 commits, and reform the changes as an amendment onto your initial commit. You can use push -f to rewrite history on your remote.