Skip to content

Conversation

@ggevay
Copy link
Contributor

@ggevay ggevay commented Jul 30, 2015

  • handle the case of a non-public default constructor
  • added a missing "return null"
  • removed some duplicating of the word "class" when printing class
    names, because class.toString also prepends it to the class name
  • fixed a typo in the documentation describing POJOs

@ggevay
Copy link
Contributor Author

ggevay commented Jul 30, 2015

I removed the added "return null", I just realized that it is not needed.

@gyfora
Copy link
Contributor

gyfora commented Jul 30, 2015

Could you please add a test case that verifies the correct behaviour?

@StephanEwen
Copy link
Contributor

Why are you changing all messages in this pull request? They were not wrong before...

@mxm
Copy link
Contributor

mxm commented Aug 7, 2015

Stephan is right. The changed messages just add noise to the pull request which makes it harder to review.

@ggevay ggevay force-pushed the analyzePojoFix branch 2 times, most recently from 90431c3 to 2f4640f Compare August 7, 2015 19:53
@ggevay
Copy link
Contributor Author

ggevay commented Aug 7, 2015

They print things like
Class class malom.GameState must have a default constructor to be used as a POJO.
If you think that the double "class" should be left the way it is, then I'm sorry for changing them. I have now removed the changes from the PR.

I have also added a test.

@StephanEwen
Copy link
Contributor

Removing the double "class" is actually not bad. Was hard to get this from the diff alone.

BTW: Simpler way may be to change the aClass.toString() call to aClass.getCanonicalName().

@ggevay
Copy link
Contributor Author

ggevay commented Aug 11, 2015

OK, I re-added the changes to remove the double "class". (Note, that the 3rd bullet in the opening comment refers to this doubling.)

Copy link
Contributor

Choose a reason for hiding this comment

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

if(defaultConstructor != null && Modifier.isPublic(defaultConstructor.getModifiers()) seems to be more readable to me but your approach is fine too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, I have changed it.

@mxm
Copy link
Contributor

mxm commented Aug 12, 2015

Your changes look good to merge.

…actor.analyzePojo

Also changed some prints which printed the word "class" twice,
because class.toString also prepends it to the class name.
@asfgit asfgit closed this in a41bc8c Aug 12, 2015
asfgit pushed a commit that referenced this pull request Aug 12, 2015
…actor.analyzePojo

Also changed some prints which printed the word "class" twice,
because class.toString also prepends it to the class name.

This closes #960.
@mxm
Copy link
Contributor

mxm commented Aug 12, 2015

Thanks for your contribution!

nikste pushed a commit to nikste/flink that referenced this pull request Sep 29, 2015
…actor.analyzePojo

Also changed some prints which printed the word "class" twice,
because class.toString also prepends it to the class name.

This closes apache#960.
pnowojski pushed a commit that referenced this pull request Jul 3, 2024
[hotfix] remove public scope for tests in cc-flink-schema-registry
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants