Skip to content

Conversation

adiChoudhary
Copy link

added a boolean check to log blacklisted features

@adiChoudhary adiChoudhary requested review from a team as code owners September 23, 2023 18:35
@CLAassistant
Copy link

CLAassistant commented Sep 23, 2023

CLA assistant check
All committers have signed the CLA.

@Zabuzard Zabuzard linked an issue Sep 23, 2023 that may be closed by this pull request
@Zabuzard Zabuzard added enhancement New feature or request priority: low labels Sep 23, 2023
Copy link
Member

@Zabuzard Zabuzard left a comment

Choose a reason for hiding this comment

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

good stuff, thanks 😄 and welcome to the project 👍

return !featureIdentifierBlacklist.contains(featureId);
boolean isBlackListed = featureIdentifierBlacklist.contains(featureId);
if (isBlackListed) {
logger.info(String.format("%s is blacklisted 😥", featureId));
Copy link
Member

Choose a reason for hiding this comment

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

  • use the loggers own formatting api, not urs (otherwise the string is also computed if info is disabled) - logger.info("{} is blacklisted", featureId)
  • id change the text slightly for better UX into Feature {} is disabled.

Copy link
Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@Alathreon Alathreon left a comment

Choose a reason for hiding this comment

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

An idea would be make so the blacklist itself filter the features, so logging would be easier:
isEnabled would be left untouched, and a new method would be added

class FeatureBlacklist<T> {
  ...
  public <F> Stream<F> disableMacthing(Stream<F> features, Function<T, F> idExtractor) {
    return features.filter(f -> {
      T id = idExtractor.apply(f);
      if(this.isEnabled(id)) {
        return true;
      } else {
        logger.info("{} is blacklisted 😥", id);
        return false;
      }
    });
  }
}

Features.java use case:

return blacklist.disableMacthing(features.stream(), Feature::getClass).toList();

Code actions use case:

List<CodeAction> codeActions =
        blacklist.disableMacthing(
          Stream.of(new FormatCodeCommand(), new EvalCodeCommand(jshellEval),
          f -> f.getClass().getName()
        ).toList();

@adiChoudhary
Copy link
Author

@Alathreon incorporating the above change would introduce changes in class
image
This would require refactoring of connected classes. Should we do that
image
was thinking of creating a simple decorator to add logging functionality
image
We can do something like above also, this would allow us to easily supply any filtration logic, keep our logging intact

@Alathreon
Copy link
Contributor

Alathreon commented Sep 24, 2023

@adiChoudhary

incorporating the above change would introduce changes in class

F isn't a generic that you put on the class, but I indeed forgot to declare it correctly, I modified my review message, it should be declared on the method directly.

This would require refactoring of connected classes. Should we do that

No

We can do something like above also, this would allow us to easily supply any filtration logic, keep our logging intact

This code is the same as the previous isEnabled idea, which we decided to not follow, so no

@adiChoudhary
Copy link
Author

@Alathreon, @Zabuzard added suggested changes, please review

@Alathreon
Copy link
Contributor

@adiChoudhary you didn't reverse the changes you did to isEnabled, and did you verify that it works ?

@adiChoudhary
Copy link
Author

sorry thats why i was seeing 2 log statements, will do that

@adiChoudhary
Copy link
Author

Reverted it back now

Copy link
Contributor

@java-coding-prodigy java-coding-prodigy left a comment

Choose a reason for hiding this comment

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

Since we won’t be using isEnabled outside of the disableMatching method, maybe we could in-line and remove it?

public boolean isEnabled(T featureId) {
return !featureIdentifierBlacklist.contains(featureId);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add Java doc here.


FeatureBlacklist<Class<?>> blacklist = blacklistConfig.normal();
return features.stream().filter(f -> blacklist.isEnabled(f.getClass())).toList();
return blacklist.disableMatching(features.stream(), Feature::getClass).toList();
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this should be made Object::getClass

return !featureIdentifierBlacklist.contains(featureId);
}

public <F> Stream<F> disableMatching(Stream<F> features, Function<F, T> idExtractor) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should use Stream<? extends F> and Function<? super F, ? extends T> as it allows more flexibility

@ankitsmt211
Copy link
Member

There's a new PR #980 that seems to be targetting the same issue, i assume this is not active anymore? Perhaps a polite reminder to author of new PR to check issues with maintainers beforehand. Otherwise there work might be a waste of effort.

@marko-radosavljevic
Copy link
Contributor

Is this PR nearly done, or it should be closed in favour of #980? Both seem to be inactive, and this one is at least passing all checks.

I mean, this looks fine, maybe slightly overcomplicated since project should be readable by beginners, but I think we are long past that point hah. It's just a few lines anyway, seems like there is not a good reason to drag this out for 2 more months. ^^

@Taz03
Copy link
Member

Taz03 commented Mar 16, 2024

merged #1048

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request priority: low

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Disabled Features should be logged

8 participants