Skip to content

Conversation

molexx
Copy link
Contributor

@molexx molexx commented Jan 12, 2019

Allow the @GraphQLDescription to be placed on getter methods as well as fields, and search for it if it is in a different place to the JPA annotations.
As well as allowing flexibility in developer's/teams preferences this enables support for Entity generation tools such as HibernateTools.

@igdianov
Copy link
Collaborator

@molexx Thank you for PR. It looks good to me.

I would just like ask if you can make a separate test entity class for testing annotation on methods so the tests cover both use cases?

@igdianov
Copy link
Collaborator

@molexx The reason is that I would like to keep unit tests and associated entities isolated from each other so that other folks PRs don't start clashing.

@codecov
Copy link

codecov bot commented Jan 25, 2019

Codecov Report

Merging #63 into master will increase coverage by 0.08%.
The diff coverage is 67.64%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #63      +/-   ##
============================================
+ Coverage     60.75%   60.83%   +0.08%     
- Complexity      250      258       +8     
============================================
  Files            21       21              
  Lines          1595     1629      +34     
  Branches        249      258       +9     
============================================
+ Hits            969      991      +22     
- Misses          521      529       +8     
- Partials        105      109       +4
Impacted Files Coverage Δ Complexity Δ
...jpa/query/schema/impl/GraphQLJpaSchemaBuilder.java 85.61% <67.64%> (-1.86%) 70 <5> (+8)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df38b86...2d7f756. Read the comment docs.

@molexx
Copy link
Contributor Author

molexx commented Jan 26, 2019

I think I've covered all use cases - method annotations are used in Droid and CodeList, field annotations are used in Character.

I was trying to create minimal clutter, but sure: I'll duplicate the model classes to a new package and revert the changes in the existing ones.

@igdianov
Copy link
Collaborator

@molexx Thank you! You covered all the use cases. Great PR! I will merge it with the changes.

@molexx
Copy link
Contributor Author

molexx commented Feb 2, 2019

I'm trying to split a separate set of models out to my own package and it's getting quite messy. If every PR makes its own copy of the test models the repo is going to get very cluttered.

Creating a copy of the models in com.introproventures.graphql.jpa.query.schema.model.flexibledescription doesn't work because all @Entitys packaged below StarwarsQueryExecutorTests get discovered by the EntityManager scanning so we have duplicate Entity/table names.

com.introproventures.graphql.jpa.query.flexibledescriptionschema avoids that but starts to create a very untidy package tree. Then on test initialization data.sql expects all the tables to be created including author, book etc. which I don't need to use as Droid, Character and CodeList cover all the test cases.

IMHO it's not worth this effort and clutter and would be better to efficiently reuse the existing models where possible as I have already done.

I can push ahead if you really want, if so please advise how to organise the packaging.

@igdianov
Copy link
Collaborator

igdianov commented Feb 2, 2019

@molexx No worries. I will merge as is. It is important that the tests are there. We can refactor if needed. Thank you!

@igdianov igdianov merged commit 2df2a46 into introproventures:master Feb 2, 2019
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