BlessCSS / bless

CSS Post-Processor
blesscss.com
MIT License
282 stars 60 forks source link

Move css-band-aid fork to bless #63

Closed mtscout6 closed 8 years ago

mtscout6 commented 8 years ago

Brings the latest changes from css-band-aid back to bless. This converts the src from CoffeeScript to ES6 using Babel as its transpiler.

Should address #44, #37, #45, #38, #33, #27, and #8

It doesn't auto merge at the moment, but this PR does contain a lot of changes...

paulyoung commented 8 years ago

The v4 branch is currently comprised of the following:

The importer, parser, and reporter all have specs and it would be great not to lose those.

mtscout6 commented 8 years ago

I think those are new since we forked, I'll take some time to review those and see how well they port over.

paulyoung commented 8 years ago

It would be good if everything in src/ (maybe not cli stuff) had an equivalent spec file.

I think the documentation / comments on v4 are valuable too.

mtscout6 commented 8 years ago

Ideally I would think that once this is merged to the v4 branch, we just merge it to master and cut the v4 alpha release, then iterate from there. It would be nice to have the v4 release out within the next week. Then we wouldn't have to worry about explicit v4 docs since it would be the latest published version.

mtscout6 commented 8 years ago

I agree about the testing aspect, we could have done more unit tests as opposed to integration there than we did.

I think the parser tests are encapsulated here:

They could use some better names, but they are both higher level integration tests.

We didn't get around to adding tests for the count reporter so yes that is also lacking at the moment.

mtscout6 commented 8 years ago

I'm thinking that reporters will need a little more thought since the cli tool is doing two different commands and both would need different reporters. I like your direction to make the reporter more pluggable; I think it's worth looking at but I think that could happen in a follow on PR.

paulyoung commented 8 years ago

I like your direction to make the reporter more pluggable

That decision was guided by being able to write unit tests for an external reporter

I think it's worth looking at but I think that could happen in a follow on PR

Could we create issues for any regressions?

aabenoja commented 8 years ago

I still see the value in our current integration tests, so perhaps we could migrate those over an integration folder for now. I agree that there is an absence of low-level unit tests, but I don't think those should stop an alpha release. We'll create an issue/multiple issues for the missing unit tests.

mtscout6 commented 8 years ago

@paulyoung Do you have anything else you wish to address with this?

paulyoung commented 8 years ago

Do we need to commit the source maps in the fixtures? I can't imagine they're being used for anything.

paulyoung commented 8 years ago

It doesn't look like it so I think we should probably remove and ignore them.

mtscout6 commented 8 years ago

They are needed, we are using them as expected output when source maps are turned on from the cli engine here: https://github.com/css-band-aid/css-band-aid/blob/master/test/cli-chunker.js#L41

They are generally obscure so they have been checked manually, then we just check that the output remains the same as the last time we manually checked that the source maps were working.

mtscout6 commented 8 years ago

Do you have any other concerns @paulyoung?

paulyoung commented 8 years ago

@mtscout6 :ship:

mtscout6 commented 8 years ago

Merged to master in 7cd3a194518b7eaa65b9f8b12c897356feb45a39