Skip to content

updated to support EntityCombinedScreen #3#6

Open
genthalili wants to merge 4 commits into
balvi:masterfrom
genthalili:master
Open

updated to support EntityCombinedScreen #3#6
genthalili wants to merge 4 commits into
balvi:masterfrom
genthalili:master

Conversation

@genthalili

Copy link
Copy Markdown
Contributor

Hi,

I'm proposing a solution to support EntityCombinedScreen, please feel free to update at my forked repo.

@Zynde

Zynde commented May 16, 2018

Copy link
Copy Markdown

Oh great. Thanks. I've not had the time to go back and do this myself.

@bresche

bresche commented May 16, 2018

Copy link
Copy Markdown
Contributor

Thanks for the pull request. I looked at the code and it has duplicates. It would be better to move the code into super class (AbstractAnnotationDispatcherBean). Could you create some unit tests?

@genthalili

Copy link
Copy Markdown
Contributor Author

Hi @bresche ,

Could you explain more how to avoid this duplicated code?

@bresche

bresche commented May 16, 2018

Copy link
Copy Markdown
Contributor

Pull up Member from BrowseAnnotationDispatcherBean,CombinedAnnotationDispatcherBean to AbstractAnnotationDispatcherBean and make methods generic:

protected Collection<T> getSupportedCombinedAnnotationExecutors(Annotation annotation) {
      Collection<BrowseAnnotationExecutor> annotationExecutors = getCombinedAnnotationExecutors();
      return (Collection<T>) getSupportedAnnotationExecutors(annotationExecutors, annotation);
  }

@genthalili

Copy link
Copy Markdown
Contributor Author

Removed duplicated code and added tests, however tests are not working, I have a Null exception, could you assis on this ?

@genthalili

Copy link
Copy Markdown
Contributor Author

@bresche, any suggestion to fix this ?

@bresche

bresche commented May 17, 2018

Copy link
Copy Markdown
Contributor

I'll take a look at it.

@bresche

bresche commented May 17, 2018

Copy link
Copy Markdown
Contributor

Hi, I put some mocks in your test spec. Now you can use them in spock style and create some good test cases.

@genthalili

Copy link
Copy Markdown
Contributor Author

Thanks!
I think you covered the basic testings, that's super! Can we consider merging this pull request ?

@bresche

bresche commented May 18, 2018

Copy link
Copy Markdown
Contributor

Unfortunately, not quite. We need more tests. The CombinedAnnotationDispatcherBean class is not well tested and contains code which is commented. I have slightly increased the test code coverage.

@Zynde

Zynde commented Jun 5, 2018

Copy link
Copy Markdown

Has this progressed any further?

@bresche

bresche commented Jun 6, 2018

Copy link
Copy Markdown
Contributor

There are still 2 classes to test.

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.

3 participants