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

RuleSet into (ActiveRuleSet, RuleSet) based on their responsibility #336

Merged
merged 89 commits into from
Jun 6, 2022

Conversation

Chibikuri
Copy link
Member

@Chibikuri Chibikuri commented Jan 6, 2022

This change is Reviewable

@Chibikuri Chibikuri changed the title RuleSet into (ActiveRuleSet, Rule) RuleSet into (ActiveRuleSet, RuleSet) based on their responsibility Jan 6, 2022
@zigen
Copy link
Contributor

zigen commented Jan 9, 2022

@Chibikuri don't forget to merge the latest master

@Chibikuri
Copy link
Member Author

@Chibikuri don't forget to merge the latest master

Thanks, I did.

@Chibikuri Chibikuri marked this pull request as ready for review May 10, 2022 10:45
@Chibikuri Chibikuri requested a review from zigen May 30, 2022 19:32
Copy link
Contributor

@zigen zigen left a comment

Choose a reason for hiding this comment

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

I'm not sure the ConnectionManager::respondToRequest is fine. it's time to refactor it.

Reviewed 1 of 23 files at r3, 8 of 74 files at r4, all commit messages.
Reviewable status: 9 of 107 files reviewed, 1 unresolved discussion (waiting on @Chibikuri and @zigen)


quisp/modules/QRSA/ConnectionManager/ConnectionManager.cc line 335 at r4 (raw file):

   * ...
   */
  std::map<int, std::vector<std::unique_ptr<Rule>>> rules_array;

it's a map, but it's named an array...

Copy link
Contributor

@zigen zigen left a comment

Choose a reason for hiding this comment

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

:lgtm:

let's merge it, and then refactor it. we cannot do refactor in this PR

Reviewable status: 9 of 107 files reviewed, 1 unresolved discussion (waiting on @Chibikuri and @zigen)

Copy link
Member Author

@Chibikuri Chibikuri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 9 of 107 files reviewed, 1 unresolved discussion (waiting on @zigen)


quisp/modules/QRSA/ConnectionManager/ConnectionManager.cc line 335 at r4 (raw file):

Previously, zigen (Kentaro "zigen" Teramoto) wrote…

it's a map, but it's named an array...

Done.

Copy link
Member Author

@Chibikuri Chibikuri left a comment

Choose a reason for hiding this comment

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

Got it!

Reviewable status: 9 of 107 files reviewed, 1 unresolved discussion (waiting on @zigen)

Copy link
Member Author

@Chibikuri Chibikuri left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 9 of 107 files reviewed, 1 unresolved discussion (waiting on @zigen)

Copy link
Contributor

@zigen zigen left a comment

Choose a reason for hiding this comment

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

I forgot to resolve the discussion.
:lgtm:

Reviewable status: 9 of 107 files reviewed, all discussions resolved (waiting on @zigen)

Copy link
Contributor

@zigen zigen left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 9 of 107 files reviewed, all discussions resolved (waiting on @zigen)

Copy link
Contributor

@zigen zigen left a comment

Choose a reason for hiding this comment

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

Reviewed 29 of 85 files at r1, 3 of 23 files at r3, 65 of 74 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Chibikuri)

@Chibikuri Chibikuri merged commit 8e1f821 into master Jun 6, 2022
@Chibikuri Chibikuri deleted the active_ruleset_dev branch August 30, 2022 07:45
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.

2 participants