Skip to content

Conversation

@kgdiem
Copy link
Contributor

@kgdiem kgdiem commented Sep 27, 2016

Pull request for #25

Development checklist (To be completed by main developer)

  • Code is working as expected
  • Acceptance criteria met
  • Passes coding standards
  • Unit tests written and passing
  • PR open and linked to issue
  • PR using correct template
  • Issue closed
  • A quick self code review performed

Code Review Checklist

Code

  • No syntax/runtime errors and warnings in the code
  • No deprecated functions in the code
  • “Dead Code” should be removed. If it is a temporary hack, it should be identified as such.
  • All code follows coding standard (preferably https://github.com/airbnb/javascript)
  • No magic numbers. There should be no magic numbers like 6,10 etc… any numbers like this should be defined as a constant. And Constants should be well commented about the purpose.
  • Above criteria met

Documentation

  • All methods are commented in clear language. If it is unclear to the reader, it is unclear to the user.
  • All class, variable, and method modifiers should be examined for correctness.
  • Describe behaviour for known input corner-cases.
  • Complex algorithms should be explained with references. For example, document the reference that identifies the equation, formula, or pattern. In all cases, examine the algorithm and determine if it can be simplified.
  • Incomplete code is marked with //TODO or //FIXME markers.
  • All public and private APIs are examined for updates.
  • Above criteria met

Unit Tests

  • Unit tests are added for each code path, and behaviour.
  • Unit tests must cover error conditions and invalid parameter cases.
  • Unit tests for standard algorithms should be examined against the standard for expected results.
  • Ensure that the code fixes the issue, or implements the requirement, and that the unit test confirms it. If the unit test confirms a fix for issue, add the issue number to the doc block.
  • Unit tests must have assertions
  • Regression tests for fixes are included.
  • As a reviewer, you should understand the code. If you don’t, the review may not be complete, or the code may not be well commented.
  • Above criteria met

Error Handling

  • Invalid parameter values are handled properly early in methods (Fast Fail/Return Early).
  • Don't swallow exceptions! You should at least log the exception.
  • Above criteria met

Testing checklist

  • All unit tests pass
  • All acceptance criteria are met and working as expected
  • Spot checks have been performed on existing functionality that may have been affected

@lottspot
Copy link
Contributor

Please remember to merge PRs into develop rather than master

@lottspot lottspot changed the base branch from master to develop September 27, 2016 14:33
Copy link
Contributor

@lottspot lottspot left a comment

Choose a reason for hiding this comment

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

I have a feeling that you didn't intend to make all of these changes. I would recommend that you create a new working branch which is based on develop rather than master and re-apply the changes you were trying to make to that branch.

@kgdiem
Copy link
Contributor Author

kgdiem commented Sep 27, 2016

My b. I didn't realize you merged my last branch to develop.

I'll fix it later.

@kgdiem
Copy link
Contributor Author

kgdiem commented Sep 28, 2016

I was going to start fixing this, but was wondering if, rather than jsdoc, we should use something like wicked https://github.com/thlorenz/wicked that will populate our repo's wiki.

There are also jsdoc plugins to do this:
https://github.com/thlorenz/jsdoc-githubify
https://github.com/jsdoc2md/jsdoc-to-markdown

@kgdiem
Copy link
Contributor Author

kgdiem commented Sep 28, 2016

Also, should we decide to use jsdoc, should a config file and NPM task be included in a separate PR?

ex

 "scripts": {
    "start": "node app.js",
    "jsdoc": "jsdoc -c jsdoc.json -r"
  },

@lottspot
Copy link
Contributor

I think it would be awesome to have something like that! I would love to have our documentation be able to update and automatically land in our GitHub wiki in some sort of minimal effort way.

Personally, I would rather use a jsdoc plugin than a replacement for jsdoc so that our entire docs solution doesn't become married to this one feature (plus it seems a bit silly to throw our jsdoc for something that uses it under the hood)

That being said, it's defnitely more of a "nice to have" than a "must have", so I don't think we should block work on the API to incorporate GH wiki integration for our docs. If you want to do some testing on the side though with those jsdoc plugins, I'm all for it!

But yeah, we should at some point add an npm script to do doc generation either way.

As far as this PR is concerned though, I think at this point we should just close it. Since the working branch it's being merged from was based on master instead of develop, any attempt to make this PR mergable into develop is going to be more headaches than it's worth. Just start a clean working branch based on develop and make any changes you want merged on the new working branch.

@kgdiem
Copy link
Contributor Author

kgdiem commented Sep 28, 2016

Don't mean to keep commenting on a closed thread but I'd rather get started with jsdoc right away, retroactive documentation is no fun whatsoever lol.

@lottspot
Copy link
Contributor

lottspot commented Sep 28, 2016

Yeah sorry, I probably didn't communicate what I meant very clearly--

jsdoc has already been added to the project. We can generate API docs from day 1. What I was saying is getting one of the plugins working to automatically push the generated documentation into our GH wiki pages is not necessarily essential. The essential part is that we're getting the methods documented inline (which we really wouldn't even need to install jsdoc to do anyways).

Until we have time to get a jsdoc plugin working to integrate with our GH wiki we can just generate the regular jsdoc html pages and push them to a gh-pages branch or something like that.

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.

4 participants