Skip to content
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.

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

Support excluding filter on mapper scan feature #836

Merged
merged 15 commits into from
May 10, 2024

Conversation

apdya
Copy link
Contributor

@apdya apdya commented Jul 6, 2023

fix issue #548. add field ComponentScan.Filter[] excludeFilters() in @MapperScan. Support excluding filter on
mapper scan feature.

apdya and others added 2 commits July 6, 2023 15:37
@apdya apdya closed this Jul 8, 2023
@apdya apdya reopened this Jul 8, 2023
@hazendaz
Copy link
Member

hazendaz commented Jul 8, 2023

Few issues I see here

  • Add code into deprecated code
  • Adding aspectj but doesn't use it at all
  • Alignment issues (ie adding properties that are against the next line javadocs)

@kazuki43zoo Will need to check this out but please cleanup items noted and make a case for why deprecated code is being filled out while deprecation was left. Not seeing that was even needed off hand but didn't look deeper than that.

@apdya
Copy link
Contributor Author

apdya commented Jul 9, 2023

Few issues I see here

  • Add code into deprecated code
  • Adding aspectj but doesn't use it at all
  • Alignment issues (ie adding properties that are against the next line javadocs)

@kazuki43zoo Will need to check this out but please cleanup items noted and make a case for why deprecated code is being filled out while deprecation was left. Not seeing that was even needed off hand but didn't look deeper than that

Few issues I see here

  • Add code into deprecated code
  • Adding aspectj but doesn't use it at all
  • Alignment issues (ie adding properties that are against the next line javadocs)

@kazuki43zoo Will need to check this out but please cleanup items noted and make a case for why deprecated code is being filled out while deprecation was left. Not seeing that was even needed off hand but didn't look deeper than that.

thanks for replying

  • issue 1 and 3: I will modify my code and update this pr.
  • issue 2: ComponentScan.Filter[] is from spring-context, it supports ASPECTJ FilterType, which need dependency on aspectjweaver, and this dependency is set as optional in spring-context, so this dependency need be added.

pom.xml Show resolved Hide resolved
@hazendaz
Copy link
Member

hazendaz commented Jul 9, 2023

thanks for updates, few more comments.

@apdya
Copy link
Contributor Author

apdya commented Jul 11, 2023

thanks for review
I have modified my code as your comment, and explain the reason to import aspectjweaver in that conversation.

@hazendaz hazendaz requested a review from kazuki43zoo July 12, 2023 01:58
@hazendaz
Copy link
Member

@kazuki43zoo Can you take a look at this one?

@hazendaz
Copy link
Member

@apdya Tests don't build, can you please take a look?

@apdya
Copy link
Contributor Author

apdya commented Jul 12, 2023

@apdya Tests don't build, can you please take a look?

sorry I haven't run all test in my local project before commit. I update the new added test code just now, and all test passed.

@kazuki43zoo
Copy link
Member

Can you take a look at this one?

Just wait a few time!

@kazuki43zoo kazuki43zoo self-assigned this Jul 15, 2023
Copy link
Member

@kazuki43zoo kazuki43zoo left a comment

Choose a reason for hiding this comment

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

I added some comments.

@apdya

  • Could you support this enhancement for the <mybatis:scan>(XML namespace feature)? If you cannot support it, I support it.

  • Please perform ./mvnw clean verify (can apply code formatter and update license header)

@kazuki43zoo kazuki43zoo added the enhancement Improve a feature or add a new feature label Jul 15, 2023
@kazuki43zoo kazuki43zoo added this to the 3.0.3 milestone Jul 15, 2023
@kazuki43zoo kazuki43zoo added the backport to other version Backport to other maintenance version label Jul 15, 2023
@kazuki43zoo kazuki43zoo changed the title fix issue 548. Support excluding filter on mapper scan feature Jul 15, 2023
@apdya
Copy link
Contributor Author

apdya commented Jul 15, 2023

I added some comments.

@apdya

  • Could you support this enhancement for the <mybatis:scan>(XML namespace feature)? If you cannot support it, I support it.
  • Please perform ./mvnw clean verify (can apply code formatter and update license header)

@kazuki43zoo
thanks for review
I have modify my code as your comment, and updated this pr.
I would like to support XML namespace feature if this task is not urgent.
mvnw seems not work in my IDEA, i will figure it out soon. and I have add license header to all the new added file.

@apdya
Copy link
Contributor Author

apdya commented Jul 16, 2023

hi @kazuki43zoo

I have add some code to support exclude filter feature with xml. the new added element in xsd keep unified with Spring.

@apdya
Copy link
Contributor Author

apdya commented Jul 18, 2023

still need some work

@apdya
Copy link
Contributor Author

apdya commented Jul 18, 2023

still need some work

with latest commit, exclude-filter has been supported in mybatis:scan and test in org.mybatis.spring.filter.xml.XmlScanFilterTest

@apdya apdya requested a review from kazuki43zoo July 24, 2023 02:05
@kazuki43zoo kazuki43zoo modified the milestones: 3.0.3, 3.0.4 Oct 23, 2023
@kazuki43zoo kazuki43zoo removed the backport to other version Backport to other maintenance version label Nov 25, 2023
@apdya
Copy link
Contributor Author

apdya commented Apr 17, 2024

@kazuki43zoo Is there anything else I need to change?

@hazendaz
Copy link
Member

@apdya Can you fix the merge conflict that exists now?

@apdya
Copy link
Contributor Author

apdya commented May 2, 2024

@apdya Can you fix the merge conflict that exists now?

@hazendaz happy to receive your reply, merge conflict has been resolved. whether this pr can be merged into the master branch now?

@apdya apdya requested a review from hazendaz May 2, 2024 07:27
@hazendaz
Copy link
Member

hazendaz commented May 5, 2024

@apdya I'll try to get to this in a week or two. I believe all was good and just got missed. I've got a reminder to review it again and take care of merging it in. If you could, assuming there isn't one, open an issue that same support needs added to xml namespace as I see commented on here a few times so its not missed in future and at least helps others see that support is not present.

@hazendaz
Copy link
Member

hazendaz commented May 5, 2024

as to a release as I'm sure that will come next after merging, probably end of month we could also get a release out.

@coveralls
Copy link

coveralls commented May 5, 2024

Coverage Status

coverage: 90.232% (+0.9%) from 89.305%
when pulling b7f3f35 on apdya:scanfilter
into afd3335 on mybatis:master.

@apdya
Copy link
Contributor Author

apdya commented May 6, 2024

@apdya I'll try to get to this in a week or two. I believe all was good and just got missed. I've got a reminder to review it again and take care of merging it in. If you could, assuming there isn't one, open an issue that same support needs added to xml namespace as I see commented on here a few times so its not missed in future and at least helps others see that support is not present.

@hazendaz thanks for reply. <mybatis:exclude-filter >(XML namespace feature) had been supported with commit [82ef68f] in this pr. test case: org.mybatis.spring.filter.xml.XmlScanFilterTest

@hazendaz hazendaz merged commit 55a5ac8 into mybatis:master May 10, 2024
12 of 13 checks passed
@hazendaz
Copy link
Member

@apdya Merged. I'll try to get a release towards end of the month. If I miss that please do remind me here :)

@apdya
Copy link
Contributor Author

apdya commented Jun 27, 2024

@apdya Merged. I'll try to get a release towards end of the month. If I miss that please do remind me here :)
@hazendaz sorry for the late reminder, i got the time wrong :(

@hazendaz
Copy link
Member

@apdya Sorry for delays. I had a few personal issues that took up most my time last month. So thank you for reminder. I'll work to get this released next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve a feature or add a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants