HackerOne users: Testing against this community violates our program's Terms of Service and will result in your bounty being denied.

Code reviews

LincLinc Admin

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:).

«1

Comments

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

    grep is your friend.

  • 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.

  • @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.)

  • @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.

  • 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.

  • 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 .

  • @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.

  • http://docs.vanillaforums.com/developers/contributing/coding-standard/
    Code MUST use 4 spaces for indenting, not tabs.

    https://github.com/vanilla/addons/blob/master/.editorconfig
    indent_style = space
    indent_size = 3

  • R_JR_J Admin

    @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 ;)

  • @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.

  • LincLinc Admin
    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_JR_J Admin

    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 Admin

    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 ✭✭✭
    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?

Sign In or Register to comment.