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

[Misc] Code hygiene #1222

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

[Misc] Code hygiene #1222

wants to merge 5 commits into from

Conversation

kenliao94
Copy link
Contributor

@kenliao94 kenliao94 commented May 13, 2024

  • Rewrote some for-loop for cleaner code.
  • Removed one of the unused import.

@jbonofre jbonofre self-requested a review May 13, 2024 08:52
@kenliao94 kenliao94 requested a review from jbonofre May 13, 2024 09:08
@kenliao94 kenliao94 changed the title [Misc] Add README.md and code hygiene [Misc] Code hygiene May 13, 2024
Copy link
Contributor

@mattrpav mattrpav left a comment

Choose a reason for hiding this comment

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

There are a bunch of varying changes here. Not sure it makes sense as a single commit. Perhaps the iteration Java-language updates go as one easy commit.

I do not see a reason to remove null checks.

Is this PR part of a JIRA or NO-JIRA change?

@kenliao94
Copy link
Contributor Author

There are a bunch of varying changes here. Not sure it makes sense as a single commit. Perhaps the iteration Java-language updates go as one easy commit.

I do not see a reason to remove null checks.

Is this PR part of a JIRA or NO-JIRA change?

This is part of a NO-JIRA change. I am just trying to clean up the code as I navigate the code :) I moved the removing redundant null checks from this PR to another I will create later to make this PR more about trivial updates.

@kenliao94 kenliao94 requested a review from mattrpav May 13, 2024 17:00
@kenliao94
Copy link
Contributor Author

Hey Matt and JB, are there any more concerns about this PR? I checked the Jenkins failing tests they are about KahaDB

[INFO] ActiveMQ ........................................... SUCCESS [ 0.852 s]

[INFO] ActiveMQ :: BOM .................................... SUCCESS [ 0.248 s]

[INFO] ActiveMQ :: Openwire Generator ..................... SUCCESS [ 1.091 s]

[INFO] ActiveMQ :: Client ................................. SUCCESS [02:50 min]

[INFO] ActiveMQ :: Openwire Legacy Support ................ SUCCESS [ 2.411 s]

[INFO] ActiveMQ :: JAAS ................................... SUCCESS [01:46 min]

[INFO] ActiveMQ :: Broker ................................. SUCCESS [01:18 min]

[INFO] ActiveMQ :: KahaDB Store ........................... FAILURE [ 03:30 h]

And the broker tests passed.

@jbonofre
Copy link
Member

I did a new pass and it looks good to me. If @mattrpav is OK (as he requested a change), I will merge it.

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