8000 Large python style multiline value fix by r3db34n · Pull Request #210 · go-ini/ini · GitHub
[go: up one dir, main page]
Skip to content

Large python style multiline value fix - #210

Merged
unknwon merged 23 commits into
go-ini:masterfrom
r3db34n1an:large-python-style-multiline-value-fix
Nov 6, 2019
Merged

Large python style multiline value fix#210
unknwon merged 23 commits into
go-ini:masterfrom
r3db34n1an:large-python-style-multiline-value-fix

Conversation

@r3db34n
@r3db34n r3db34n commented Oct 4, 2019 •
edited
Loading
Copy link
Copy Markdown
Contributor

What problem should be fixed?

This change

Have you added test cases to catch the problem?

There is a new test file with test cases as well as a test data file to cover empty lines in Python-style multiline values.

@r3db34n
r3db34n commented Oct 4, 2019
Copy link
Copy Markdown
Contributor Author

Builds succeeds on all go versions except 1.9.x and 1.10.x because of test harness dependency goconvey (used by go-ini/ini):

$ go get github.com/smartystreets/goconvey
# golang.org/x/tools/go/gcexportdata
../../../golang.org/x/tools/go/gcexportdata/gcexportdata.go:103:12: undefined: gcimporter.IExportData
The command "go get github.com/smartystreets/goconvey" exited with 2.

unknwon
unknwon requested changes Oct 4, 2019
View reviewed changes
@unknwon unknwon left a comment •
edited
Loading
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the PR!

Here is my first round review comments, I try to keep number of comments low, so it would be easier for you to look at these changes first, before we discuss more things.

Comment thread ini.go Resolved
Comment thread ini_multiline_value_fix_test.go Resolved
Comment thread ini_multiline_value_fix_test.go Resolved
Comment thread ini_multiline_value_fix_test.go Resolved
Comment thread ini_multiline_value_fix_test.go Resolved
Comment thread ini_multiline_value_fix_test.go Resolved
Comment thread ini_multiline_value_fix_test.go Resolved
Comment thread parser.go Resolved
@unknwon
unknwon commented Oct 4, 2019
Copy link
Copy Markdown
Member

I don't know what's going with 1.9 and 1.10, we can ignore it for now.

@unknwon

This comment has been minimized.

Sign in to view
@unknwon

This comment has been minimized.

Sign in to view
unknwon
unknwon requested changes Oct 4, 2019
View reviewed changes
@unknwon unknwon left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the follow up! Here are my second round review comments.

Comment thread ini_python_multiline_test.go Resolved
Comment thread ini_python_multiline_test.go Resolved
Comment thread parser.go Resolved
Comment thread parser.go Resolved
Comment thread parser.go Resolved
Comment thread parser.go Resolved
Comment thread parser.go Resolved
Comment thread parser.go Resolved
Comment thread parser.go Resolved
Comment thread parser.go Resolved
@unknwon
unknwon commented Oct 5, 2019
Copy link
Copy Markdown
Member

Looks like CI is passing now, maybe was the flakiness of Travis CI.

@r3db34n
r3db34n requested a review from unknwon October 5, 2019 00:04
@r3db34n
r3db34n commented Oct 6, 2019
Copy link
Copy Markdown
8BD0
Contributor Author

Is there anything else you need me to do to accept the pull request?

unknwon
unknwon requested changes Oct 6, 2019
View reviewed changes
Comment thread ini.go Resolved
Comment thread ini.go Resolved
Comment thread parser.go Resolved
Comment thread parser.go Resolved
@unknwon
unknwon commented Oct 6, 2019
Copy link
Copy Markdown
Member

This PR is ready to merge without the "debug thing". Thanks again for the work, but it's your call :)

@r3db34n
r3db34n commented Oct 6, 2019
Copy link
Copy Markdown
Contributor Author

This PR is ready to merge without the "debug thing". Thanks again for the work, but it's your call :)

I will make those changes you requested. Let me know how you want to proceed with the logging

@r3db34n
r3db34n commented Oct 6, 2019
Copy link
Copy Markdown
Contributor Author

OK, all changes have been made as requested. Should be good to go now

unknwon
unknwon reviewed Oct 6, 2019
View reviewed changes
Comment thread ini.go Resolved
Comment thread ini.go Resolved
unknwon
unknwon requested changes Oct 7, 2019
View reviewed changes
Comment thread ini_python_multiline_test.go Resolved
Comment thread parser.go Resolved
Comment thread ini.go Resolved
Comment thread ini.go Resolved
Comment thread parser.go Resolved
Comment thread parser.go Resolved
Comment thread parser.go Resolved
Comment thread parser.go Resolved
Comment thread parser.go Resolved
Comment thread parser.go Resolved
@r3db34n
r3db34n commented Oct 26, 2019
Copy link
Copy Markdown
Contributor Author

I will make those changes as agreed, but this will be my final round of modifications. If you want further changes, I would suggest you merge the code and make the changes in your repo.

@r3db34n
r3db34n requested a review from unknwon October 27, 2019 01:03
@unknwon
unknwon commented Nov 1, 2019
Copy link
Copy Markdown
Member

I feel GitHub review is broken... so I'll just list links of two comments:

@unknwon
unknwon commented Nov 6, 2019
Copy link
Copy Markdown
Member

Thank you! Merging.

@unknwon
unknwon merged commit 0f135c9 into go-ini:master Nov 6, 2019
@unknwon unknwon mentioned this pull request Nov 9, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@unknwon unknwon unknwon approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@r3db34n @unknwon
0