Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Strip spaces from resources (fixes #148) #223

Merged
merged 3 commits into from
Mar 5, 2018

Conversation

leplatrem
Copy link
Contributor

Fixes #148

@leplatrem leplatrem requested a review from glasserc March 2, 2018 11:10
@leplatrem leplatrem force-pushed the 148-strip-spaces-from-resources branch from 981e0a2 to 3d63435 Compare March 2, 2018 11:11
Copy link
Contributor

@glasserc glasserc left a comment

Choose a reason for hiding this comment

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

Personally, I find "any whitespace separates elements" to be clearer than "only newlines separate elements, and spaces are ignored between elements". But the code looks fine.

@leplatrem
Copy link
Contributor Author

leplatrem commented Mar 2, 2018 via email

@glasserc
Copy link
Contributor

glasserc commented Mar 2, 2018

I don't find it easier to read. Instead of seeing three things, my eye sees six things, and I just have to know that some of them are joined together by virtue of being on a line.

@leplatrem
Copy link
Contributor Author

my eye sees six things

Should we get rid of the colons maybe?

@glasserc
Copy link
Contributor

glasserc commented Mar 5, 2018

Hmm, maybe, or maybe something more indicative of a relationship? I'd suggest a colon : instead of a semicolon, that would be sort of JSON-y, but that wouldn't work very well for stage -> preview -> signed. Maybe ->? Or is that too cutesy?

@leplatrem
Copy link
Contributor Author

Maybe ->? Or is that too cutesy?

Funny, I had suggested => in #88 (comment)

Why not then, let's go with ->. It's super easy (we'll strip it anyway)

@leplatrem leplatrem merged commit 05394e7 into master Mar 5, 2018
@leplatrem leplatrem deleted the 148-strip-spaces-from-resources branch March 5, 2018 15:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants