search

RocHack / bb

Command line Blackboard client
MIT License
72 stars 8 forks source link

Fixes parse_netrc not grabbing passwords with spaces. #24

Closed AstralSorcerer closed 9 years ago

AstralSorcerer commented 9 years ago

Closes #23.

joeljk13 commented 9 years ago

It may be a good idea to squash all these together as one commit - there's only really 1 change, not 5. But it's not that important, and I'd say the actual content is ready to merge.

clehner commented 9 years ago

+1 squash - we should probably get in the habit of doing that more (when merging into master).

Can the two sed commands be combined into one? i.e. sed '/%TOKEN%/!d; regex1; regex2'

AstralSorcerer commented 9 years ago

Good idea...I'm still a bit of a git noob. Can I do that without having to do a seperate pull request? And I think the sed commands can be combined, but only if the -n option is removed.

clehner commented 9 years ago

Yes, if you push (including with --force) to this branch, the pull req will update. See https://github.com/RocHack/bb/pull/21#issuecomment-70446509 for some info about squashing commits. Yes, starting the sed command with /.../!d removes the need for -n.

AstralSorcerer commented 9 years ago

OK, will do!

AstralSorcerer commented 9 years ago

That what you were looking for?

clehner commented 9 years ago

You've got some merge conflict thingies in there but it looks like the regex is right. Consider also breaking it out it onto multiple lines if possible.

AstralSorcerer commented 9 years ago

The regex? And I'm not seeing any merge conflicts.

AstralSorcerer commented 9 years ago

Oh whoops. I must have missed that. Sorry.

AstralSorcerer commented 9 years ago

That better?

clehner commented 9 years ago

Right. breaking the regex can be done by replacing a semicolon or 2 with newlines (in regex='...'). what you did is also fine.

AstralSorcerer commented 9 years ago

OK. Thanks for all the help!

clehner commented 9 years ago

You're welcome!