sloum
/
gfu
1
1
Fork 1
Code Issues Pull Requests Releases 1 Wiki Activity

WIP Refactor to ensure carriage returns are removed, plus tests #3

Manually merged
sloum merged 3 commits from asdf/gfu:no_carriage_returns_allowed into master 2019-09-21 21:25:53 +00:00
Conversation 7 Commits 3 Files Changed 2 +233 -64
asdf commented 2019-09-18 12:45:30 +00:00
Collaborator
Copy Link

gfu was inconsistent with carriage returns, removing them whenever a line is modified but leaving them on the unmodified lines.

This change uses bufio scanner to read lines, because it strips all line terminators automatically. It has an added advantage of not having to manage End Of File state when reading.

I added some basic tests to make sure the output is correct, but this required some changes to the design of readFile(). outFile is also no longer a global variable, but is returned/accepted as required.

This needs a bit of review before consideration for merging, namely:

  • I'm fairly certain this works fine, but it seems messier or something now. I want to clean up the flags as suggested in this answer but maybe there are some new design issues?
  • I renamed functions and variables for consistency, but maybe that's not helpful. i.e. buildComments is now buildInfoText because it matches the documentation.
  • Lines are terminated with now only, and I don't think there should be an option to use . Do you agree?
gfu was inconsistent with carriage returns, removing them whenever a line is modified but leaving them on the unmodified lines. This change uses `bufio` `scanner` to read lines, because it strips all line terminators automatically. It has an added advantage of not having to manage End Of File state when reading. I added some basic tests to make sure the output is correct, but this required some changes to the design of `readFile()`. `outFile` is also no longer a global variable, but is returned/accepted as required. This needs a bit of review before consideration for merging, namely: - I'm fairly certain this works fine, but it seems messier or something now. I want to clean up the flags as suggested in [this answer](https://stackoverflow.com/questions/19761963/flag-command-line-parsing-in-golang) but maybe there are some new design issues? - I renamed functions and variables for consistency, but maybe that's not helpful. i.e. `buildComments` is now `buildInfoText` because it matches the documentation. - Lines are terminated with ` ` now only, and I don't think there should be an option to use ` `. Do you agree?
sloum reviewed 2019-09-18 15:57:07 +00:00
sloum left a comment
Owner
Copy Link

This all looks good to me. You mentioned it as a work in progress, so I will leave the PR open for now and keep an eye out for updates to it.

What do you think about the header/footer functionality? Is that something that makes sense in this application? Can you think of anything else that would be good? The only thing I can think of is a flag to direct to a different output file.... that is largely taken care of by your stdout flag though as someone can just direct into a file. Maybe listing that as an example usage would be a good call? That way those less familiar with nix style piping and redirecting can still take advantage of that as a feature?

This all looks good to me. You mentioned it as a work in progress, so I will leave the PR open for now and keep an eye out for updates to it. What do you think about the header/footer functionality? Is that something that makes sense in this application? Can you think of anything else that would be good? The only thing I can think of is a flag to direct to a different output file.... that is largely taken care of by your stdout flag though as someone can just direct into a file. Maybe listing that as an example usage would be a good call? That way those less familiar with nix style piping and redirecting can still take advantage of that as a feature?
main.go Outdated
sloum commented 2019-09-18 15:49:35 +00:00
Owner
Copy Link

So, if I am understanding correctly, this states that the default scanner behavior is scanLines, yeah? In which case this all makes sense. It is weird that they hide that fact in this method rather than mentioning it in the docs to scan itself.

So, if I am understanding correctly, [this](https://golang.org/pkg/bufio/#Scanner.Split) states that the default scanner behavior is scanLines, yeah? In which case this all makes sense. It is weird that they hide that fact in this method rather than mentioning it in the docs to scan itself.
asdf commented 2019-09-19 02:37:30 +00:00
Author
Collaborator
Copy Link

That is right. I actually found out about it while searching how to deal with both types of line termination styles, and by then using the examples.

That is right. I actually found out about it while searching how to deal with both types of line termination styles, and by then using the examples.
main.go Outdated
sloum commented 2019-09-18 15:50:39 +00:00
Owner
Copy Link

I think it would probably make sense to move the newline to errorExit itself, that way it does not have to be added to every errorExit that gets called. What do you think?

I think it would probably make sense to move the newline to `errorExit` itself, that way it does not have to be added to every errorExit that gets called. What do you think?
main.go Outdated
@ -129,2 +127,3 @@
func main() {
deconstructCommentLinks := flag.Bool("d", false, "Deconstruct a gophermap's info text lines back to plain text")
//command-line flags and responses
deconstructInfoTextLines := flag.Bool("d", false, "Deconstruct a gophermap's info text lines back to plain text")
sloum commented 2019-09-18 15:51:21 +00:00
Owner
Copy Link

I agree with changing this everywhere for consistency (from commentLinks to InfoTextLines). Good call.

I agree with changing this everywhere for consistency (from commentLinks to InfoTextLines). Good call.
sloum commented 2019-09-18 15:59:40 +00:00
Owner
Copy Link

I read the linked stack overflow page and I'm not sure why they are doing differently with flags (in the answer I mean, the question is way different from what is going on in gfu). Do you mean moving the flag variables into global space and out of main? Or something else?

I read the linked stack overflow page and I'm not sure why they are doing differently with flags (in the answer I mean, the question is way different from what is going on in gfu). Do you mean moving the flag variables into global space and out of main? Or something else?
asdf commented 2019-09-19 02:39:52 +00:00
Author
Collaborator
Copy Link

Yes, sorry, the question I linked to isn't really relevant. I meant that moving as much of the flag management out of main() would improve readability. The style that is in the answer is the approach I will try.

Also, to respond to your main review text, I agree with all your suggestions. The "roadmap" might look like:

  • add redirection documentation
  • add headers and footers functionality
  • accept second argument for an output file
  • man page and make file (to install the man page)
Yes, sorry, the question I linked to isn't really relevant. I meant that moving as much of the flag management out of `main()` would improve readability. The style that is in the answer is the approach I will try. Also, to respond to your main review text, I agree with all your suggestions. The "roadmap" might look like: - add redirection documentation - add headers and footers functionality - accept second argument for an output file - man page and make file (to install the man page)
sloum commented 2019-09-19 18:40:51 +00:00
Owner
Copy Link

Do you know much about makefiles? I really want to include a man page with bombadillo... but I dont really know make at all. Is that the right tool for that job? I am also thinking that with v2 I might try to get it together as a snap package once it is ready for release. Sorry, off topic for this repo.

That roadmap looks great :)

Do you know much about makefiles? I really want to include a man page with bombadillo... but I dont really know make at all. Is that the right tool for that job? I am also thinking that with v2 I might try to get it together as a snap package once it is ready for release. Sorry, off topic for this repo. That roadmap looks great :)
asdf commented 2019-09-20 08:09:08 +00:00
Author
Collaborator
Copy Link

I made a new issue on Bombadillo to cover this. Generally, anything I learn here I'll apply to Bombadillo as well (I started this while making changes to bombadillo-info).
To answer your question: I've only looked in to what is possible, but will have to learn any of the methods.

I made a new issue on Bombadillo to cover this. Generally, anything I learn here I'll apply to Bombadillo as well (I started this while making changes to bombadillo-info). To answer your question: I've only looked in to what is possible, but will have to learn any of the methods.
👍 1
sloum commented 2019-09-20 15:30:03 +00:00
Owner
Copy Link

Awesome.

This PR looks good to me, is it ready for merge? Or were you still adding more (I felt like I had not merged it for some reason or other relating to that).

Awesome. This PR looks good to me, is it ready for merge? Or were you still adding more (I felt like I had not merged it for some reason or other relating to that).
asdf commented 2019-09-20 21:43:28 +00:00
Author
Collaborator
Copy Link

Still working my way through it, I'll update once I have completed the work.

Still working my way through it, I'll update once I have completed the work.
👍 1
asdf commented 2019-09-21 10:42:10 +00:00
Author
Collaborator
Copy Link

OK, this PR has all intended items complete, and is ready for merge after review.

This last commit changes:

  • errors are returned back to the calling function. I understand this is more reusable, but probably overkill here.
  • main handles errors using log.Fatalln
  • command line flags moved to init() and global variables
  • comments added and updated

It was fun to make and I learned a lot.

OK, this PR has all intended items complete, and is ready for merge after review. This last commit changes: - errors are returned back to the calling function. I understand this is more reusable, but probably overkill here. - main handles errors using `log.Fatalln` - command line flags moved to `init()` and global variables - comments added and updated It was fun to make and I learned a lot.
sloum approved these changes 2019-09-21 21:25:45 +00:00
sloum left a comment
Owner
Copy Link

Looks great! I was confused about a small part but then saw what you did. Makes perfect sense (it had to do with a named return and me not noticing it so wondering why you werent returning an error... but you were).

I'm gonna merge it. You've done a great job with this! The changes will, I think, be really beneficial. :)

Looks great! I was confused about a small part but then saw what you did. Makes perfect sense (it had to do with a named return and me not noticing it so wondering why you werent returning an error... but you were). I'm gonna merge it. You've done a great job with this! The changes will, I think, be really beneficial. :)
sloum closed this pull request 2019-09-21 21:25:53 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: sloum/gfu#3
No description provided.
Delete Branch "asdf/gfu:no_carriage_returns_allowed"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?