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.

Sign up for GitHub

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

Jump to bottom

Random stylistic/idiomatic improvements. #1

Closed
wants to merge 2 commits into from
Closed

Random stylistic/idiomatic improvements. #1

wants to merge 2 commits into from

Conversation

brunal
Copy link

@brunal brunal commented Sep 27, 2016

No description provided.

@brunal
learn.py: get rid of "file exists"
a48da6d 
Difference in the saved data file: the last line will now be \n-terminated.
This has no influence over how files are loaded, it just makes the code a bit leaner.
@brunal
Update get_data.py
055f7ed 
- Rely on ensure_whereami_path() in get_train_data()
- Don't construct & merge (append, extend, +) unneeded lists
- Rely on dict comprehension syntax for a shorter sample()
kootenpv
kootenpv reviewed Sep 27, 2016
View reviewed changes
Copy link
Owner

@kootenpv kootenpv left a comment

Choose a reason for hiding this comment

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

I will need to write tests. I have the idea that the reading of files will not work unless we make sure we do not end up with a trailing newline. Did you test whether it works like this?

@kootenpv
Copy link
Owner

But a lot of thanks! (Stylistic) suggestions are very much appreciated. It is my bad that tests are lacking.

@kootenpv
Copy link
Owner

My sincerest of apologies: I did separately clone your branch and it really does seem to work! I now merged them in as suggested by github as it was conflicting by now.

I mention you in the commit message, but unfortunately there was no way for it to show your contribution! I'll make sure to properly include your pull request next time!

@kootenpv
Copy link
Owner

I only noticed it did not work after training. Had to revert the map(json.dumps) :(

@brunal
Copy link
Author

brunal commented Sep 28, 2016

My sincerest of apologies [...]
Np, my goal was to show you some style improvements more than fame.

the reading of files will not work unless we make sure we do not end up with a trailing newline
You were already getting a trailing newline for all records but the last one.

Had to revert the map(json.dumps) :(
You mean json.loads maybe? What's the error? That's quite surprising.

@kootenpv
Copy link
Owner

It had to do with the trailing newline in the end.

@kootenpv
Copy link
Owner

kootenpv commented Oct 1, 2016

Thanks once more, now closing. Feel free to open this one or to make other improvements.

@kootenpv kootenpv closed this Oct 1, 2016
@Cerv4ntez Cerv4ntez mentioned this pull request Apr 20, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kootenpv kootenpv kootenpv left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@brunal @kootenpv