Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added Parse functionnality and some unit tests #3
base: master
Are you sure you want to change the base?
Added Parse functionnality and some unit tests #3
Changes from 1 commit
f7e63d1
0962039
e193de4
320b9ff
8ea809b
57bdcc5
20273a3
2b24b84
f771020
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These documented restrictions don't appear to match the regular expression pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implenented in parse so we have same behaviour in both methodes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an FYI: This method allocates a lot of temporary memory on the heap. It is possible to write this without allocating anything other than the
List<byte>
, though the parsing has to be more manual. I can provide more details if you're interested.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
glad to see any improvement if you feel like it's worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether it's worth it depends upon the consumer's situation. If they're only parsing one short blob of text then it's probably fine as is. If they're running many operations concurrently or in an application that's sensitive to GC pauses, then the current implementation may be problematic. This is general library code, so I try to be as well behaved as possible as we cannot make many assumptions about the user's requirements.
At a high level, all of these can be avoided:
Replace
string per lineThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ParseLookup implementation seems the most promising one.
Will have a look at a state machine to implement special cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
Environment.NewLine
means you can get different behaviours on different machines for the same input. This kind of thing regularly breaks CI, for example. I'd rather see split explicitly on'\r'
and'\n'
and remove empty entries (though I'd rather not useString.Split
at all as it allocates an array and temporary strings, both of which can be avoided).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it to use StringReader and remove the ToArray() in linq query.