evertiro / historical-redux2

A simple, easily extendable options framework for WordPress based on NHP Theme Options Framework.
http://reduxframework.com
Other
105 stars 43 forks source link

Use substr to remove prefix and postfix when getting section id. #141

Closed mgmartel closed 11 years ago

mgmartel commented 11 years ago

Fixes #140.

mgmartel commented 11 years ago

Changed.

However, I doubt the usefulness of defining '_section' as a variable somewhere. As far as I can see, it is only used to create the settings section id and to associate setting fields to that section. This is only for internal use.

Only scenario this would be touched by external code is when someone uses the WP Settings API directly to add another field or something, and then there's no harm in having a consistent section id (option name + section key + '_section').

Or am I missing something?

tivnet commented 11 years ago

I did not say "variable", but "constant". const SECTION_SUFFIX = '_section';

Imagine if you keep 8 in your patch, and later someone decides to change the '_section' to '_redux_section'. That would be a hard-to-find bug. A similar bug would happen if one changes '_section' to something else - but not in all places. Same problem. With const, it won't happen. Sorry for this 101 on programming. You know all this, of course. It's just pain to see a good piece of software not polished to perfection.

mgmartel commented 11 years ago

@tivnet: No need to make this personal. Good call on using substr instead of hardcoding the 8 - corrected this already. However, the author of the framework hard-codes and repeats most used strings and currently uses no class constants at all - that is the author's choice. I see no need to change the author's programming style in a straight forward bug fix like this one.

tivnet commented 11 years ago

Of course, that was a general comment. Sorry if I offended you. Wasn't my intent.

evertiro commented 11 years ago

@mgmartel To be fair, the issue you brought up in regards to my coding style is one I'm trying to rectify with V3.0.0 (see #138). You have to remember that I originally forked Redux from NHP, and despite the enhancements I've made and the significant amount of code reworking I've done, some of the original author's coding styles have endured.

@all I'm always trying to improve and polish Redux, and I certainly welcome any and all suggestions, tips, tricks, and pull requests you care to throw my way. Redux has become more popular than I had ever hoped when I initially forked it, and it's because of all of you that I have the motivation to continue its production. Come on everyone! Let's build the ultimate options framework!

tivnet commented 11 years ago

@ghost1227 : that comment was mine, not @mgmartel, and I remember that you forked from NHP, so it's not about YOUR style.

mgmartel commented 11 years ago

@all I think we're all contributing to this framework because we think it's pretty awesome! Keep up the good work @ghost1227 !

evertiro commented 11 years ago

@tivnet Ok, maybe I took it a bit more personally than I should have :smile:

@mgmartel Thanks!