Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Code Style Guide etc inc f to g #1522

Merged
merged 2 commits into from
Feb 27, 2015
Merged

Code Style Guide etc inc f to g #1522

merged 2 commits into from
Feb 27, 2015

Conversation

phil-davis
Copy link
Contributor

No description provided.

rbgarga
rbgarga reviewed Feb 26, 2015
View reviewed changes
etc/inc/filter_log.inc
( ($flent != "") && ( is_array($filtertext)) && (match_filter_field($flent, $filtertext)) ) ) {
if (!$filterinterface || ($filterinterface == $flent['interface'])) {
if ((($flent != "") && (!is_array($filtertext)) && (match_filter_line ($flent, $filtertext))) ||
(($flent != "") && ( is_array($filtertext)) && (match_filter_field($flent, $filtertext)))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is another case as I commented out on filter.inc. I'll re-review #1515, #1519 and #1520 and look for similar cases

@phil-davis
Copy link
Contributor Author

Yeh, when I did a global "convert leading spaces to tabs" in notepad++ it converts every 4 spaces to a tab. Then it displays (at least for PHP for me) a tab as 4 spaces. So in places like this there is no difference in the appearance in the editor.
Of course in an editor or diff visualizer (Github GUI diff display) that is rendering a tab as 8 spaces, it looks obvious that there is a difference.
I will have a look through these 2 pull requests and fixup...

@jim-p
Copy link
Contributor

jim-p commented Feb 26, 2015

@phil-davis In notepad++, activate View > Show Symbol > Show Whitespace and Tab, it renders tabs as a red arrow and spaces as a red dot.
I use a similar mode in Ultraedit, it makes spotting those sorts of things easy. Also using 8 for a tab stop helps. At one point we had that listed somewhere as the recommended tab size.

@rbgarga
Copy link
Member

rbgarga commented Feb 26, 2015

Since pfSense uses K&R style, the best is to set editor to show tabs in 8-spaces size

@rbgarga
Copy link
Member

rbgarga commented Feb 26, 2015

There are tools like https://github.com/squizlabs/PHP_CodeSniffer that could be useful too, but looks like there is not any standard like K&R there

More style guide changes
918bdf0 
In gwlb.inc at line 676 and 779 I added an extra set of brackets. In the
"if" clause as a whole there were a mix of && and || used that were
relying on the PHP standard that && has precedence over ||
In actual fact the original code should have been working fine, the
extra brackets make sure that the intention is clear.
@phil-davis
Copy link
Contributor Author

I checked this for multi-line if statements and fixed it up. Also found a couple more single-line "if" that I had missed. It should be good to go now.

@BBcan177
Copy link
Contributor

I use Ultraedit also and prefer it over Notepad++

What about changing "<>" to "!=" to keep everything consistent?

@netgate-git-updates netgate-git-updates merged commit 918bdf0 into pfsense:master Feb 27, 2015
netgate-git-updates pushed a commit that referenced this pull request Feb 27, 2015
@rbgarga
Merge pull request #1522 from phil-davis/Code-Style-Guide-etc-inc-f-to-g
fd74eee
@phil-davis phil-davis deleted the Code-Style-Guide-etc-inc-f-to-g branch February 27, 2015 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@phil-davis @jim-p @rbgarga @BBcan177 @netgate-git-updates