Skip to content

Simplify setup, add failing test, code coverage #1

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

Merged
merged 3 commits into from
Feb 10, 2016

Conversation

briancavalier
Copy link
Member

Just thinking out loud here, and all of this is totally up for discussion.

Two of the biggest changes are:

  1. switching to the name index.js to avoid the two mv commands. I'm not a huge fan of that name, but I also don't mind it. In this case, it makes life a bit easier. Thoughts?
  2. making the test fail by default 😈. My thinking is that this forces someone to at least look at it, although, maybe it's too heavy handed?

@briancavalier
Copy link
Member Author

Added code coverage with isparta. I'd rather use nyc, but it's coverage reports are broken. Maybe something like this would work, or we can wait for a fix and then switch.

@briancavalier briancavalier changed the title Add suggestions for discussion Simplify setup, add failing test, code coverage Feb 9, 2016
@TylorS
Copy link
Member

TylorS commented Feb 9, 2016

I don't know anything about code coverage, so I think that if this works for now I'm all for it.

@briancavalier
Copy link
Member Author

Yeah, agreed. I say we just go with isparta and keep an eye on nyc.

Any thoughts on index.js and making the test fail by default?

cc @davidchase

@davidchase
Copy link
Member

The index.js seems like good convention... as it does make life easier we use that in our current project...

I think we can use what works for test coverage... i think nyc was done initially because of ava... could be wrong

what is the idea for failing the test by default? is that for consumer to be a good citizen before submitting their packages?

@briancavalier
Copy link
Member Author

I think we can use what works for test coverage

👍

is that for consumer to be a good citizen before submitting their packages?

Yeah, although now I think it's too heavy-handed :) Since we'll have test coverage, that's probably enough.

@TylorS
Copy link
Member

TylorS commented Feb 9, 2016

index.js seems okay. That's what I was going to do originally, but it seemed to be of convention to name the 'main' file the same as the package name. e.g. @most/dom-event -> dom-event.js

Regarding the tests, I think having them fail is okay, if someone really doesn't want to write tests they could delete them.

@briancavalier
Copy link
Member Author

Ok, I tweaked a few things. Let me know what you think.

  1. Test passes by default now. I say we just pick pass or fail and run with it. I picked pass.
  2. Test uses function exported from index.js
  3. Removed reference to @most/core, since it doesn't exist (yet)

@TylorS
Copy link
Member

TylorS commented Feb 10, 2016

I think it all looks good

@briancavalier
Copy link
Member Author

Any other thoughts, @davidchase? Good to merge?

@davidchase
Copy link
Member

yeah i think its good to merge... if you guys dont mind when i have sometime i still would like to tackle the ava and co setup just to see what i can come up with and see if any of that is viable both for curiosity and potential use in the project

@TylorS
Copy link
Member

TylorS commented Feb 10, 2016

I have no issues with at least trying to move to ava in the future. It's always worth trying potentially better things. 👍

Merge and transfer to the mostjs org?

@briancavalier
Copy link
Member Author

i still would like to tackle the ava and co setup

Sure 👍

Merge and transfer to the mostjs org?

👌 go for it!

TylorS added a commit that referenced this pull request Feb 10, 2016
Simplify setup, add failing test, code coverage
@TylorS TylorS merged commit 697ffae into mostjs:master Feb 10, 2016
@TylorS
Copy link
Member

TylorS commented Feb 10, 2016

Merged and transferred

@briancavalier
Copy link
Member Author

💥

@briancavalier briancavalier deleted the add-suggestions branch February 10, 2016 17:12
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.

3 participants