The feature to fix 'em all!

[emilgoldsmith]

This feature should provide the tools to close #54, fix #34, and resolve #13.

The feature to be implemented in this PR is the one discussed in #68. And I will open another discussion issue later to talk about how the final API should look like and edit it in here.

The main points needed to be implemented for the full feature is:

• actually parsing the comments
• finalize the API (decide which labels to include and how to substitute for each of them)
• connect parsing and the API
• write thorough tests including the tests added in Add failing tests for #13 #16 (and therefore close Add failing tests for #13 #16)
• Document everything thoroughly in the README

[emilgoldsmith]

This is currently just a dummy PR to outline the main points needed to be implemented. I may or may not find time to actually work on it this weekend, I'll probably finalize #67 first, and I have a few other things I want to work on as well.
Opening this PR also allows other people to take a stab at implementing it if they feel like it, it's definitely on my to-do list though but anyone is free to beat me to it :).

[coveralls]

Coverage remained the same at 99.371% when pulling 774f405 on feature/label-interpolations-by-comment into 2786011 on master.

[coveralls]

Coverage decreased (-0.5%) to 98.942% when pulling 6cd11e1 on feature/label-interpolations-by-comment into 6c0873f on master.

[emilgoldsmith]

Proof of concept for the shortened commands + probably even the final implementation of it implemented @mxstbr @ismay. It would be way overkill using a trie or something with better complexity when we'll only have 10 commands or something like that to check against, so I think this should work just fine, right? :)

[coveralls]

Coverage decreased (-0.5%) to 99.505% when pulling b4d2595 on feature/label-interpolations-by-comment into d7d0122 on 1.0.

[emilgoldsmith]

Okay, we're nearly there!

I just need a tiny bit of feedback, specifically concerning the handling of whitespacee in the comments of #87, and then I'd also like to hear your opinion on what we should do if someone writes an invalid tag, as for example /* sc-invald */, should we throw an error and let the user know they errored, or should we just ignore it and implement our default, silently failing?

I think I prefer the first with letting them know as the fact they prefix with sc- must mean they want to use our API and in that case I feel like we should let them know if they made a typo etc. which would create unexpected behaviour.

Other than that I just need to polish a bit of code, polish / add a few more tests, and update the README.
I'm assuming I'll just add the docs to our README initially and then we can move it to the website soon after, yeah?

EDIT:
About the errors, that would also include ambiguously shortened values (as in incorrectly shortened)

[coveralls]

Coverage decreased (-0.5%) to 99.531% when pulling e5ba26e on feature/label-interpolations-by-comment into d7d0122 on 1.0.

[ghost]

what we should do if someone writes an invalid tag, as for example /* sc-invald */, should we throw an error and let the user know they errored, or should we just ignore it and implement our default, silently failing?

Hmm, not sure. As in an actual error or a stylelint error?

[emilgoldsmith]

Hmm, not sure. As in an actual error or a stylelint error?

Yeah that's also a relevant question which my initial thought at least was to throw a real error, because I thought it was a bit weird making custom stylelint errors, but maybe that could actually work I guess. We could call the rule styled-components/interpolation-tag or something like that

[ghost]

If you make it a custom stylelint rule it'd be something we could probably neatly solve by including it in stylelint-config-styled-components-processor

[emilgoldsmith]

If you make it a custom stylelint rule it'd be something we could probably neatly solve by including it in stylelint-config-styled-components-processor

Yeah that'd be one way. I feel like it would create a bit of a dangerous possible mismatch between versions though, it would be a very strong peerDependency anyway in terms of if the API changes we need to change the rule.
That'd also mean that the rule wasn't static which I guess is suboptimal.

The other way I thought was to maybe to inject a stylelint error if we went down the stylelint error path. I'm not sure exactly how you would inject it but I'd hope it was possible before the final part where we were previously removing errors from the result (before we had the config repo), though that would of course definitely be possible.

[ghost]

I feel like it would create a bit of a dangerous possible mismatch between versions though, it would be a very strong peerDependency anyway in terms of if the API changes we need to change the rule.

That's true. Although in the worst case scenario it'd be false positives, or it not warning when it should (if you allow the processor to just fail silently).

The other way I thought was to maybe to inject a stylelint error if we went down the stylelint error path. I'm not sure exactly how you would inject it but I'd hope it was possible before the final part where we were previously removing errors from the result (before we had the config repo), though that would of course definitely be possible.

That should be possible. But I'd prefer not to rely on the internal structure of the stylelintResult. I'd say that if we want it to be reported through stylelint we should do it with a rule. But yeah, that also has its downsides (as you say, the rule could change for example).

Another way would be to throw an error from our processor, but I don't know how stylelint would handle that (if it would crash, or something else).

[emilgoldsmith]

Another way would be to throw an error from our processor, but I don't know how stylelint would handle that (if it would crash, or something else).

Yeah that was my initial idea, and I assumed it would crash, which as I said:

I think I prefer the first with letting them know as the fact they prefix with sc- must mean they want to use our API and in that case I feel like we should let them know if they made a typo etc. which would create unexpected behaviour.

And I therefore thought that justified crashing stylelint? But yeah, as we both agree there are ups and downs of the different choices, and throwing an error I guess could also do something else than crash stylelint? Though I would be surprised and can't imagine what it would do then.

[ghost]

Hmm yeah. I think all things considered I'd then also prefer throwing an error from our processor.

One thing I am worried about is whether editors with stylelint linters will restart stylelint when it crashes, but that maybe isn't really our concern.

It seems like the best first option, and we could always adapt it should it not work as expected.

[mxstbr]

Can't wait to land this! Let's put docs into the README for now, then write and publish an article, release 1.0 sometime this week and then work on moving the docs?

[emilgoldsmith]

It seems like the best first option, and we could always adapt it should it not work as expected.

Yeah I think that's probably the main point. Let's start out with throwing an error, and if it screws with editors (which is a very valid point) we can adapt it. Maybe use the rule or something of the like.

[emilgoldsmith]

Can't wait to land this! Let's put docs into the README for now, then write and publish an article, release 1.0 sometime this week and then work on moving the docs?

Sounds like a plan :).

The final feedback I need from you guys is just regarding my last comment in #87, as I think we settled the error thing for now. And then I'll try and make some time in the next few days to finish up this PR :).

[coveralls]

Coverage decreased (-0.5%) to 99.539% when pulling 9bd805c on feature/label-interpolations-by-comment into d7d0122 on 1.0.

[emilgoldsmith]

Woot woot, I think I'm done! I'm pretty happy with it now :). I'll just work on the README now and maybe do a bit of squashing of commits for nicer history, and I'm ready for review and merge to our v1.0 branch :).

[coveralls]

Coverage remained the same at 100.0% when pulling acf6499 on feature/label-interpolations-by-comment into d7d0122 on 1.0.

[coveralls]

Coverage remained the same at 100.0% when pulling b7a3a60 on feature/label-interpolations-by-comment into d7d0122 on 1.0.

[emilgoldsmith]

Tiny spelling error in the readme. Can I still push a commit to fix it or are you already squashing commits etc.?

Just finished now, you can add it now if you want and I can squash it again? Also feel free to take some more time to give the whole thing another review if you want @ismay

[ghost]

Just finished now, you can add it now if you want and I can squash it again? Also feel free to take some more time to give the whole thing another review if you want @ismay

Ok done. It's a supertiny fix, but anyway :). Other than that no remarks 👍

[emilgoldsmith]

Ok done. It's a supertiny fix, but anyway :). Other than that no remarks 👍

Yep and I just squashed that one in as well :). So let's wait for the CI to pass and then merge it in :)

[coveralls]

Coverage remained the same at 100.0% when pulling 86b660f on feature/label-interpolations-by-comment into d7d0122 on 1.0.

[coveralls]

Coverage remained the same at 100.0% when pulling 86b660f on feature/label-interpolations-by-comment into d7d0122 on 1.0.

[coveralls]

Coverage remained the same at 100.0% when pulling 86b660f on feature/label-interpolations-by-comment into d7d0122 on 1.0.