Skip to content
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

Add NSURL init method #5

Merged
merged 12 commits into from
Aug 16, 2016
Merged

Add NSURL init method #5

merged 12 commits into from
Aug 16, 2016

Conversation

phoney
Copy link
Contributor

@phoney phoney commented Aug 2, 2016

This pull request adds a simple init method that takes a file NSURL to the csv file.

Unit tests and playground were updated.

Also, update unit tests and playground.
Calling code can optionally specify the line ending. If they don't specify the line ending then it will be sniffed automatically. By passing the correct line ending to the streamReader() no line ending characters are included in the lines returned from the streamReader. Note that any files with mixed line endings will result in undefined behavior. But it was always like that.
If the correct lineEnding is set for the streamReader no line ending characters will be returned as part of lines. So there's no need to check for them in the line contents.
@Jeehut
Copy link
Member

Jeehut commented Aug 4, 2016

Thanks, looks good at first glance, will have a final look soon and merge. 👍

@Jeehut
Copy link
Member

Jeehut commented Aug 15, 2016

I just had a look into this one, too. And although it looks good I can't merge it as it has conflicts with one of your other MRs that I merged. Could you rebase this MR so I can merge?

@phoney
Copy link
Contributor Author

phoney commented Aug 16, 2016

I think it's ready now.

I think you can close issues 1, 3, 4.

I have more improvements for performance and perhaps memory usage. I wanted to wait until these PRs were accepted. Do you have a good way to test memory usage?

@Jeehut
Copy link
Member

Jeehut commented Aug 16, 2016

Thanks. True, I was gonna check and close those issues right before I release a new version, but I plan to do this soon anyway so gonna close them in a minute. ;)

As for automatic testing of the memory usage, I think this is still a missing feature which Xcode should provide. Maybe we get this with Xcode 9 or 10? At least I don't know a way of testing this in a good way automatically, I think we have to stick with improving the memory leaking parts manually and making sure to comment the affected parts in code well so no-one changes them back to memory-leaking code.

@Jeehut Jeehut merged commit 684c747 into FlineDev:stable Aug 16, 2016
@Jeehut
Copy link
Member

Jeehut commented Aug 16, 2016

@phoney I just gave you write permissions on this project so you can work directly within it using work/ branches. You should also be able to accept merge requests and close issues if appropriate. Please make sure not to merge anything that doesn't comply with the contribution guidelines.

Also please don't merge your own changes, even if all tests pass – it is always good to wait until somebody has reviewed your changes. Thanks again for helping improve this project! 👍

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