Skip to content

Better filenames tracking - #153

Merged
benhoyt merged 20 commits into
benhoyt:masterfrom
xonixx:better-filenames-tracking
Oct 2, 2022
Merged

Better filenames tracking#153
benhoyt merged 20 commits into
benhoyt:masterfrom
xonixx:better-filenames-tracking

Conversation

@xonixx

@xonixx xonixx commented Sep 22, 2022

Copy link
Copy Markdown
Contributor

This is the other PR in context of #144.

This is re-implemenation of #150 based on work in #151. I think #151 can be just closed.

Basically this is almost equivalent to #151 plus:

  1. added tests
  2. added doc comments
  3. applied minor optimization

@xonixx

xonixx commented Sep 26, 2022

Copy link
Copy Markdown
Contributor Author

@benhoyt Any thoughts?

@benhoyt

benhoyt commented Sep 26, 2022

Copy link
Copy Markdown
Owner

Yes, sorry, last week was a busy week. I'll try to get to this tonight or tomorrow.

@benhoyt benhoyt left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for this. Looks good overall! Requesting a few minor changes and some additional test cases.

Comment thread internal/parseutil/filereader.go
Comment thread internal/parseutil/filereader.go
Comment thread internal/parseutil/filereader.go Outdated
Comment thread internal/parseutil/filereader.go Outdated
Comment thread internal/parseutil/filereader.go Outdated
Comment thread internal/parseutil/filereader.go Outdated
Comment thread internal/parseutil/filereader_test.go Outdated
Comment thread internal/parseutil/filereader_test.go Outdated
Comment thread internal/parseutil/filereader_test.go Outdated
Comment thread internal/parseutil/filereader_test.go Outdated
@xonixx

xonixx commented Sep 30, 2022

Copy link
Copy Markdown
Contributor Author

Done. Please check if it's ok now.

@benhoyt
benhoyt merged commit 0b251f1 into benhoyt:master Oct 2, 2022
@benhoyt

benhoyt commented Oct 2, 2022

Copy link
Copy Markdown
Owner

Merged, thanks!

@xonixx xonixx mentioned this pull request Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants