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

Node and Javascript guidelines file #59

Closed
RichardLitt opened this issue Oct 19, 2015 · 11 comments · Fixed by #92
Closed

Node and Javascript guidelines file #59

RichardLitt opened this issue Oct 19, 2015 · 11 comments · Fixed by #92
Assignees

Comments

@RichardLitt
Copy link
Member

We should have a nodejs and javascript guidelines file to link to from all IPFS js repositories. I can't seem to find a good example of any JS guidelines in our current repositories (anyone know of one?), so I think we should hash out what standards we want here, so that we can add that to an eventual file, like we did with Go in #58.

Open questions:

  • Standard format? Do we want to conform to ferross/standard?
  • Standard testing framework?
  • Standard test coverage needed?

I could build us a comprehensive styleguide given time if needed; plenty of good examples on RichardLitt/awesome-styleguides to work from.

cc @diasdavid @jbenet

@dignifiedquire
Copy link
Member

Some suggestions (most of these are currently used in either js-ipfs-api or station)

  • Build system: npm run (small) gulp larger projects
  • Testing node: mocha
  • Testing browser: karma + mocha
  • Browser building: webpack or browserify (this fight is still ongoing ;))
  • Linting: eslint + eslint-config-standard (same as ferros/standard, but easier to integrate and customize if needed)

Also I'm suggesting to set up automated release process + auto generated changelogs in the same style as I have done for karma: https://github.com/karma-runner/karma/blob/master/gruntfile.js#L143-L152

@dignifiedquire
Copy link
Member

Also I forgot, greenkeeper all the repos!

@harlantwood
Copy link
Contributor

+1 to all of the above. Also would be nice to measure test coverage, would be a good column for ipfs-inactive/project-repos#1 :) are there other code quality metrics we'd like to measure?

RichardLitt added a commit that referenced this issue Dec 27, 2015
This should close #59
@jbenet jbenet added the status/in-progress In progress label Dec 27, 2015
@RichardLitt
Copy link
Member Author

Put these notes into a file. @harlantwood, not that I can think of.

@harlantwood
Copy link
Contributor

istanbul seems to be the popular choice for measuring test coverage, iff we want to do that.

@dignifiedquire
Copy link
Member

👎 for coverage, I've got more experience with that than I'd prefer, and it ends up taking a lot of time to set up and not being very beneficial in my experience.

RichardLitt added a commit that referenced this issue Jan 7, 2016
This should close #59
@jbenet jbenet removed the status/in-progress In progress label Jan 10, 2016
@daviddias
Copy link
Member

coverage is nice to have and lets us know how much code we had that we don't check, we don't need to enforce 100% code coverage (for now)

@harlantwood
Copy link
Contributor

I haven't tried it, but perhaps https://coveralls.io/ makes test coverage reporting easier... It seems quite popular and is free for open source.

@dignifiedquire
Copy link
Member

@harlantwood it's nice for displaying the stats but you still have to do all the hard work of actually getting your test suite to generate those coverage reports

@jbenet
Copy link
Member

jbenet commented Jan 13, 2016 via email

@chriscool
Copy link
Contributor

I don't know much about the js tools. In general I think it is nice to be able to check that all or most of the big features have at least a few tests. On the other hand I agree that it's probably not worth it to try to have numbers to be able to say for example that 85% of the code paths are tested.

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 a pull request may close this issue.

6 participants