Howdy, Stranger!

It looks like you're new here. If you want to get involved, click one of these buttons!

Try Vanilla Forums Cloud product

Code reviews

LincLinc Director of DevelopmentDetroit Vanilla Staff
edited February 2015 in Development

I'm willing to code review your plugin/application if you desire, given some criteria:

  • You use Vanilla's coding standard.
  • Your plugin is public on GitHub, in our directory, and follows our licensing policy.
  • Only give me one to review at a time (i.e. don't drop all 15 plugins you've made on my head at once).
  • You have recently spruced up the plugin (i.e. no lazy hand-offs of things you haven't yourself reviewed recently).
  • You are very patient with me.

I'm also willing to review specific changesets if you set them up as a pull request on your repo.

I'll leave feedback as issues and/or line notes. I will not be offended if you ignore some or all of my feedback. I make no guarantees my code review will be as thorough as you wish or catch every issue in one pass. I'll also call out lapses in coding standards so you might wanna cleanup a little first. :)

If you'd like to participate, link me to your repo here. If there are multiple plugins in the repo, please link me directly to the one you want reviewed. If I get inundated I might close the discussion until I catch up so this doesn't become "Linc's backlog of shame" (I have enough of those already, thxuverymuch :dizzy:).

R_JShadowdare

Comments

  • The idea of the code review is for approval or just general?

    grep is your friend.

  • LincLinc Director of Development Detroit Vanilla Staff

    Just general, mostly for folks who haven't done them before or who are interested in coding feedback. I don't really wanna be the gatekeeper of official approvals; I don't think I have the time or energy for that.

  • That's what I was concerned about that you were loading yourself up.

    Btw have you considered doing what phpBB does, which run a set basic requirement test before an extension can be uploaded? (They also approve but you don't have to do that). These test obviously don't cover everything but they cover the basic requirements/conformity, as well as some helpful notices (which aren't fails). It can be run against a git repo, and you can download it and run it too.

    grep is your friend.

    Adrian
  • LincLinc Director of Development Detroit Vanilla Staff

    @x00 said:
    Btw have you considered doing what phpBB does, which run a set basic requirement test before an extension can be uploaded?

    I wasn't aware of that program. Sounds like a nice front-line check.

  • Check it out. I say I up'd my game becuase of it. I've gone a bit overboard, but that is never a bad thing.

    They had a similar thing for their previous mod system, but that was even more necessary so they learned from that.

    grep is your friend.

  • The point is people don't want to fail approval, so they run he checks themselves get it all tip top.

    grep is your friend.

  • What do you think about harnessing the power of the community and opening a code review category here? (Maybe hidden from the recent discussions or for developers only.)

    My themes: pure | minusbaseline - My plugins: CSSedit | HTMLedit | InfiniteScroll | BirthdayModule | [all] - PM me about customizations

  • hgtonighthgtonight ∞ · New Moderator

    @Bleistivt said:
    What do you think about harnessing the power of the community and opening a code review category here? (Maybe hidden from the recent discussions or for developers only.)

    I think github or crucible or any other code review tool would be easier than on here since you want to attach your comments to a specific line/block of code.

    I think it would be a good idea to have community code reviews, but I think this thread is more for "this is what @linc thinks about your code in context of garden plugins."

    Search first

    Check out the Documentation! We are always looking for new content and pull requests.

    Click on insightful, awesome, and funny reactions to thank community volunteers for their valuable posts.

  • R_JR_J Cheerleader & Troubleshooter Munich Moderator

    I would welcome it just the informal way @Linc offered his service. I bet we will get a lot of insight into best practices around Vanilla coding.

  • vrijvlindervrijvlinder Papillon-Sauvage MVP

    It would be great if there was a code beautifier for Vanilla , then you just enter your crappy looking code and it is fixed automatically .

  • LincLinc Director of Development Detroit Vanilla Staff

    @vrijvlinder said:
    It would be great if there was a code beautifier for Vanilla , then you just enter your crappy looking code and it is fixed automatically .

    You can use PhpStorm to automatically fix some of your code formatting via our editorconfig and the rest can be detected by setting up CodeSniffer.

  • chanhchanh OngETC.com - CMS Researcher ✭✭
  • R_JR_J Cheerleader & Troubleshooter Munich Moderator

    @Linc said:
    You can use PhpStorm to automatically fix some of your code formatting via our editorconfig and the rest can be detected by setting up CodeSniffer.

    I don't know how much work it is to set up a custom "Fixer" for PHP-CS-Fixer but it would be great if that would also be available =)

    Even better would be to stick to PSRxy in order to benefit from all the comforts standards come with ;)

  • hgtonighthgtonight ∞ · New Moderator

    @R_J said:
    I don't know how much work it is to set up a custom "Fixer" for PHP-CS-Fixer but it would be great if that would also be available =)

    I found one somewhere, since I work in netbeans. I will see if I can dig it up.

    Even better would be to stick to PSRxy in order to benefit from all the comforts standards come with ;)

    IIRC, it is based on PSR with a few style changes.

    Search first

    Check out the Documentation! We are always looking for new content and pull requests.

    Click on insightful, awesome, and funny reactions to thank community volunteers for their valuable posts.

    R_J
  • LincLinc Director of Development Detroit Vanilla Staff
    edited March 2015

    @hgtonight said:
    IIRC, it is based on PSR with a few style changes.

    Yes, it's PSR-2 with the Lord Brackos exception ("same line opening braces" - which, as far as I've seen, is the norm amongst PHP projects anyway. We made up the name, not the rule).

    R_J
  • R_JR_J Cheerleader & Troubleshooter Munich Moderator

    So I'll try the code fixer with PSR2 as soon as I can and I'll report back how it works for me.

  • x00x00 MVP
    edited March 2015

    Whilst coding standard is important, it far from he most important thing as far as pre-checks are concerned. Though I do appreciate if someone is to review, then it needs to be to their satisfaction, to be worth their while.

    Vanilla had a sort standard, then it completely changed, and I wonder if this has been entirely without consequence. I was kind of puzzled why such a drastic change now, but whatever it is done.

    phpBB has pre-check but these test if the extension actually works. Looking at the standard of the extensions they aren't all following phpBB core standards, and I have already gone further.

    We all have our personal hates. I hate tab indentation, and phpBB standard uses that. I went half way with new line brackets, but I can't stand tabs. There is a way in git of switch between the two, so if is an issue I'll use that.

    grep is your friend.

  • R_JR_J Cheerleader & Troubleshooter Munich Moderator

    It has put all brackets in separate rows, but hasn't converted a function name, and the missing space between the function definition and the parameters (public function FunctionName($Sender). It also ignored the indenting. So it really has changed the brackets (which were just the way I wanted them to be).

    => PHP-CS-Fixer doesn't work as expected for me :(

  • rbrahmsonrbrahmson "You may say I'm a dreamer / But I'm not the only one" NY ✭✭✭
    edited May 2016

    Hi @R_J, you prompted me to look at standards and that lead to " PHP-CS-Fixer" which I was not able to install in windows. Saw several reports on the web about it not working on Windows.
    1. Is Windows your platform?
    2. What did you mean by "doesn't work as expected for me"
    3. Is there an alternative?

  • R_JR_J Cheerleader & Troubleshooter Munich Moderator

    It relies on the environment variables and they can be a mess. I have had problems with installing php under "program files" and I think the space has been the problem.
    So I would advice to do the following:
    Uninstall php, including pear and code sniffer.
    Delete all environment variable that include the old php path (not only the path variable, bat also the pear specific variables)

    If uninstalling php is not an option, make sure that all variables are correct!
    Reboot Windows since changing environment variables often do not work as expected before the system is restarted.

    a) Go to http://windows.php.net/download and download php 5.6 Non Thread Safe x64
    b) Unzip it to c:\php
    c) Install PEAR by downloading http://pear.php.net/go-pear.phar and running php go-phear.phar
    d) Install CodeSniffer: pear install PHP_CodeSniffer
    e) Try phpcs.bat --standard=PSR2 -n yourfile.php

    If that is successful, add the Vanilla directory from the repository to the CodeSniffers standards and rename Vanilla/Tests to Vanilla/Sniffs

    I really have no clue what I was saying last year...

Sign In or Register to comment.