Skip to content

updated to support EntityCombinedScreen #3#6

Open
genthalili wants to merge 4 commits intobalvi:masterfrom
genthalili:master
Open

updated to support EntityCombinedScreen #3#6
genthalili wants to merge 4 commits intobalvi: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
Copy link
Copy Markdown

Zynde commented May 16, 2018

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

@bresche
Copy link
Copy Markdown
Contributor

bresche commented May 16, 2018

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
Copy link
Copy Markdown
Contributor

bresche commented May 16, 2018

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
Copy link
Copy Markdown
Contributor

bresche commented May 17, 2018

I'll take a look at it.

@bresche
Copy link
Copy Markdown
Contributor

bresche commented May 17, 2018

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
Copy link
Copy Markdown
Contributor

bresche commented May 18, 2018

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
Copy link
Copy Markdown

Zynde commented Jun 5, 2018

Has this progressed any further?

@bresche
Copy link
Copy Markdown
Contributor

bresche commented Jun 6, 2018

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