Remove whitespace check on -c

ascii-boxes / boxes

Command line ASCII boxes unlimited!

https://boxes.thomasjensen.com/

GNU General Public License v3.0

613 stars 75 forks source link

Remove whitespace check on -c #92

Closed Davidy22 closed 3 years ago

Davidy22 commented 3 years ago

Resolves #88

tsjensen commented 3 years ago

Thanks! I love implementations which consist of only removed code (seriously!) 😊

But I fiddled a bit with it locally, and found some quirks that we'd need to address to make this feature complete:

Add a test case which ensures that blank designs in the config file continue to be rejected. We should see must specify at least one non-empty shape per design error message.
In my local ad-hoc test, this didn't happen when the design contained a tab (probably an old bug in boxes, but now would be the time to fix it)
Add test cases that shows -c " " works for spaces and tabs, and for actually centering and right-aligning stuff.

It would be great if you can do these things as well! I understand if you don't, as it's probably more than you bargained for. In that case, just say so, and I'll merge this and add the rest myself.

Davidy22 commented 3 years ago

I suspect the tabs thing working is happening for a similar reason to why full width spaces were working, an explicit check for only ASCII " ". Don't know enough about unicode to know if there's a general whitespace flag for characters, haven't gone in yet to take a look either

Can probably get the test cases in, might be a moment

tsjensen commented 3 years ago

Ok, then I would do items 1 and 2 (the tab thing and the corresponding test case), and you can do item 3 (test cases for our new alignment capability). Sounds good?

Davidy22 commented 3 years ago

Yeah sure

Davidy22 commented 3 years ago

Ended up being a whole lot uglier than I thought it would, bash was being very finicky about a " " and I eventually just made a flag that inserts a hard " ".

tsjensen commented 3 years ago

Ok, so hopefully I've improved the test runner so that it can handle whitespace arguments.

Can you please start the test case numbers at 171, I added a test case in the previous commit about the "tab thing". Thanks!

Davidy22 commented 3 years ago

Hum, tried a bunch of stuff and eval didn't come to me. Well, my additions work without the weird workaround now, the current 170 doesn't but I'm probably missing a change on my branch.

tsjensen commented 3 years ago

Great - thanks a lot for your contribution! 🎉