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

FOGL-9228 : Support multiple rules on an asset #50

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

gnandan
Copy link
Contributor

@gnandan gnandan commented Nov 12, 2024

No description provided.

Copy link
Contributor

@MarkRiddoch MarkRiddoch left a comment

Choose a reason for hiding this comment

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

I would suggest adding a couple more unit tests

  1. The one you highlight in the Jira about rename and make sure it runs the rules for the new name and not the old after it has done the rename.
  2. A test for exclude if you have a rule after the exclude that uses the excluded asset name, just to make sure it doesn't crash.
  3. A split rule with a rule that modifies one of the assets created by the split, for example an exclude of one of the split assets

You can stay with the vector if you like, that is merely a nicety, but I think the extra unit tests are worth it and the handling of split.

At solemn point we should refactor this filter, the code has become a little out of hand with al the inline actions, but let's do this in a separate Jira rather than this one.

plugin.cpp Outdated
for (const auto& entry : assetActionMap)
{
std::regex regexPattern(entry.first);
if (std::regex_match(assetName, regexPattern))
{
return entry.second; // Return the matched AssetAction object
matchedAction.push_back(entry.second);
Copy link
Contributor

Choose a reason for hiding this comment

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

If one of the actions is an asset rename action then the will fail of course.

{
found = true;
break;
tracker->addAssetTrackingTuple(configCatName, newAssetName, string("Filter"));
Copy link
Contributor

Choose a reason for hiding this comment

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

After doing the split you should probably look for any rules defined for the newly created assets.

@MarkRiddoch
Copy link
Contributor

Another unit test to add would be a test that runs two rules with the same action. The one that is potentially a very common one would be two remove rules that remove two different datapoints in the same reading.

@gnandan
Copy link
Contributor Author

gnandan commented Nov 21, 2024

This PR also fixed FOGL-9244

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