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

Fix Jandex Type parsing for array of primitive #24274

Merged
merged 1 commit into from
Mar 16, 2022
Merged

Fix Jandex Type parsing for array of primitive #24274

merged 1 commit into from
Mar 16, 2022

Conversation

luca-bassoricci
Copy link
Contributor

@luca-bassoricci luca-bassoricci commented Mar 11, 2022

I wrote test only for broken code in core-deployment, not for Panache module where the error was found.
Just ran tests for code-deployment and test on reproducer project because I can't do a full build with tests due to QuarkusBindException.

Fix #24228

@quarkus-bot
Copy link

quarkus-bot bot commented Mar 11, 2022

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should count at least 2 words to describe the change properly
  • title should preferably start with an uppercase character (if it makes sense!)
  • title should not contain an issue number (use Fix #1234 in the description instead)

This message is automatically generated by a bot.

@luca-bassoricci
Copy link
Contributor Author

Fix for #24228

@luca-bassoricci luca-bassoricci changed the title #24228 Fix Jandex Type parsing from method descriptor for array of primitive Mar 11, 2022
@luca-bassoricci luca-bassoricci changed the title Fix Jandex Type parsing from method descriptor for array of primitive Fix Jandex Type parsing for array of primitive Mar 11, 2022
@luca-bassoricci luca-bassoricci marked this pull request as ready for review March 11, 2022 23:50
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I spotted a small typo.

@gsmet
Copy link
Member

gsmet commented Mar 13, 2022

Commits squashed.

@quarkus-bot
Copy link

quarkus-bot bot commented Mar 13, 2022

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 9adef73

Status Name Step Failures Logs Raw logs
Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs

⚠️ Errors occurred while downloading the build reports. This report is incomplete.

Copy link
Contributor

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

LGTM, if @FroMage has the time to check it it would be better as he understand the ASM part better than me.

@geoand geoand requested a review from Ladicek March 14, 2022 09:35
@gsmet
Copy link
Member

gsmet commented Mar 14, 2022

Rebased and squashed.

@Ladicek
Copy link
Contributor

Ladicek commented Mar 14, 2022

The change LGTM.

I found that Jandex's Type.create javadoc is wrong, I'll fix that on the Jandex side.

@quarkus-bot
Copy link

quarkus-bot bot commented Mar 14, 2022

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 97529a8

Status Name Step Failures Logs Raw logs
Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs

⚠️ Errors occurred while downloading the build reports. This report is incomplete.

@gsmet
Copy link
Member

gsmet commented Mar 14, 2022

Could you rebase and squash the commits? When you update from the main branch, please always use the rebase option. We don't want merge commits in the PRs.

Thanks.

@luca-bassoricci
Copy link
Contributor Author

Could you rebase and squash the commits? When you update from the main branch, please always use the rebase option. We don't want merge commits in the PRs.

Thanks.

Done

@gastaldi
Copy link
Contributor

Looks good to me, but can you squash your commits before we merge it?

@luca-bassoricci
Copy link
Contributor Author

Looks good to me, but can you squash your commits before we merge it?

Sorry, I'm relative new to git and still learning how to use.
A force push solved all problems, but I still have some concerns about using force push all the times :|

@quarkus-bot
Copy link

quarkus-bot bot commented Mar 15, 2022

Failing Jobs - Building 8063eb0

Status Name Step Failures Logs Raw logs
MicroProfile TCKs Tests Verify ⚠️ Check → Logs Raw logs

@luca-bassoricci
Copy link
Contributor Author

Build failed due to timeout error

@gastaldi gastaldi merged commit a4a723c into quarkusio:main Mar 16, 2022
@gastaldi
Copy link
Contributor

Thanks!

@quarkus-bot quarkus-bot bot added this to the 2.8 - main milestone Mar 16, 2022
@luca-bassoricci
Copy link
Contributor Author

luca-bassoricci commented Mar 16, 2022

Thanks to all you guys to let me contribute! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants