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.

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

Remove unnecessary parentheses in site components #13190

Merged
merged 4 commits into from
Dec 14, 2016
Merged

Remove unnecessary parentheses in site components #13190

merged 4 commits into from
Dec 14, 2016

Conversation

frankmayer
Copy link
Contributor

@frankmayer frankmayer commented Dec 13, 2016

Summary of Changes

  • Removed unnecessary parentheses in site components

This PR is part of a set to try to separate some of the changes done in some of my previous batch PR's for site/components, which are still on hold (#12290, #12292, #12293, #12294).

Testing Instructions

None, should not change behavior

Documentation Changes Required

None.

@@ -139,7 +139,7 @@ public function display($tpl = null)
// Check the access to the newsfeed
$levels = $user->getAuthorisedViewLevels();

if (!in_array($item->access, $levels) or ((in_array($item->access, $levels) and (!in_array($item->category_access, $levels)))))
if (!in_array($item->access, $levels) or in_array($item->access, $levels) and (!in_array($item->category_access, $levels)))
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO we should keep the paranthesis when a or and a and exists because of code redability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, maybe keep only one pair instead of both?

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, just cond1 || (cond2 && cond3)

Copy link
Contributor

Choose a reason for hiding this comment

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

personal taste ||

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not doing changes besides the parentheses in this PR, to make reveiewer's lives easier ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... and get it accepted faster

Copy link
Contributor

Choose a reason for hiding this comment

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

@dgt41 all or and / || && should IMO be normalized, but that is another PR

@andrepereiradasilva
Copy link
Contributor

I have tested this item ✅ successfully on 3b3a4b7

code review


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13190.

1 similar comment
@shur
Copy link
Contributor

shur commented Dec 14, 2016

I have tested this item ✅ successfully on 3b3a4b7

code review


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13190.

@infograf768
Copy link
Member

can't we also chnage the and and or to && and || at the same time?
I think this should be normalised.

@shur
Copy link
Contributor

shur commented Dec 14, 2016

and remove spaces here: isset( $obj->{'image_intro'} ) in two places?

@frankmayer
Copy link
Contributor Author

@infograf768 Let's do this in another PR. This also might need some discussion. I want to keep these PRs streamlined, so that they can be accepted faster.

@shur
Copy link
Contributor

shur commented Dec 14, 2016

I have tested this item ✅ successfully on ae3d4e0


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13190.

@andrepereiradasilva
Copy link
Contributor

I have tested this item ✅ successfully on ae3d4e0

code review


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13190.

@jeckodevelopment
Copy link
Member

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13190.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Dec 14, 2016
@jeckodevelopment jeckodevelopment added this to the Joomla 3.7.0 milestone Dec 14, 2016
@rdeutz rdeutz merged commit 78c7599 into joomla:staging Dec 14, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Dec 14, 2016
@frankmayer frankmayer deleted the remove-unnecessary-parentheses-in-site-components branch December 14, 2016 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants