Skip to content

Update to python3 #1084

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
Merged
merged 3 commits into from
Nov 7, 2019
Merged

Update to python3 #1084

merged 3 commits into from
Nov 7, 2019

Conversation

hjmjohnson
Copy link
Contributor

No description provided.

@hjmjohnson
COMP: Fix type mismatch ambiguity
f1a44ba 
jsoncpp/src/test_lib_json/main.cpp:354:31: error:
  implicit conversion changes signedness: 'int' to
  'std::__1::vector<Json::Value *, std::__1::allocator<Json::Value *> >::size_type'
  aka 'unsigned long') [-Werror,-Wsign-conversion]
    JSONTEST_ASSERT_EQUAL(vec[i], &array[i]);
                          ~~~ ^
@hjmjohnson
COMP: Remove shadow variable warning
d76492c 
jsoncpp/src/test_lib_json/main.cpp:2261:30: warning: declaration shadows a local variable [-Wshadow]
    Json::StyledStreamWriter writer;
                             ^
jsoncpp/src/test_lib_json/main.cpp:2237:28: note: previous declaration is here
  Json::StyledStreamWriter writer;
                           ^
@hjmjohnson
ENH: Move to requiring python 3
50671dd 
Resolves open-source-parsers#1081

See for more details:
https://devguide.python.org/devcycle/#end-of-life-branches
https://pythonclock.org/
@hjmjohnson hjmjohnson requested a review from cdunn2001 November 4, 2019 13:50
@hjmjohnson hjmjohnson self-assigned this Nov 4, 2019
@coveralls
Copy link

Coverage Status

Coverage decreased (-2.1%) to 85.944% when pulling 50671dd on hjmjohnson:update-to-python3 into 2eb20a9 on open-source-parsers:master.

cdunn2001
cdunn2001 approved these changes Nov 4, 2019
View reviewed changes
src/test_lib_json/main.cpp
@@ -350,7 +350,7 @@ JSONTEST_FIXTURE_LOCAL(ValueTest, arrayInsertAtRandomIndex) {
JSONTEST_ASSERT_EQUAL(Json::Value("index1"), array[2]);
JSONTEST_ASSERT_EQUAL(Json::Value("index2"), array[3]);
// checking address
for (int i = 0; i < 3; i++) {
for (Json::ArrayIndex i = 0; i < 3; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine, but I'm kind of curious why this error just started happening. Any idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New apple clang compiler with Mac 10.15, XCode 11.2 refused to compile the previous code due to ambiguities.

src/test_lib_json/main.cpp
@@ -2268,6 +2267,7 @@ JSONTEST_FIXTURE_LOCAL(StyledStreamWriterTest, multiLineArray) {
JSONTEST_ASSERT_STRING_EQUAL(expected, result);
}
{
Json::StyledStreamWriter writer;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand how adding a writer would remove a shadowed variable. I'm sure it's fine, but could you explain it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the original code the line 2237 shadows line 2261 below it, but is not used unit line 2270.

Scope the variable called "writer" in the blocks that need them. It may also have been OK to remove line 2261 in the original code, but I was not sure what the intent of making a new write was.

Copy link
Member

Choose a reason for hiding this comment

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

I forgot to remove line 2261 when i wrote this testcase.

hjmjohnson
hjmjohnson commented Nov 4, 2019
View reviewed changes
src/test_lib_json/main.cpp
@@ -2234,7 +2234,6 @@ JSONTEST_FIXTURE_LOCAL(StyledStreamWriterTest, writeNestedObjects) {
}

JSONTEST_FIXTURE_LOCAL(StyledStreamWriterTest, multiLineArray) {
Json::StyledStreamWriter writer;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line 2237 shadows line 2261 below, but is not used unit line 2270.

@hjmjohnson hjmjohnson mentioned this pull request Nov 4, 2019
Closed
@dota17
Copy link
Member

dota17 commented Nov 6, 2019

I‘m just a liitle curious about why coverage has dropped, and found that it's because the commit 50671dd, and now I'm more curious about why this change can influence the coverage.

@BillyDonahue
Copy link
Contributor

BillyDonahue commented Nov 6, 2019 via email

@cdunn2001 cdunn2001 merged commit a86e129 into open-source-parsers:master Nov 7, 2019
@dota17 dota17 mentioned this pull request Nov 9, 2019
Merged
dota17 added a commit that referenced this pull request Nov 9, 2019
@dota17
fix conflict (#1090)
ea0fc9a 
Fix conflict: which has been modified in #1084
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdunn2001 cdunn2001 cdunn2001 approved these changes

@dota17 dota17 dota17 approved these changes

Assignees

@hjmjohnson hjmjohnson

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hjmjohnson @coveralls @dota17 @BillyDonahue @cdunn2001