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

Improved atVersion.supportedByAutomation field handling #1182

Merged
merged 4 commits into from
Jul 31, 2024

Conversation

stalgiag
Copy link
Contributor

This PR adds a custom resolver for atVersion.supportedByAutomation that uses caching.

@stalgiag stalgiag changed the title Improved supported by automation field Improved atVersion.supportedByAutomation field handling Jul 30, 2024
Copy link
Contributor

@howard-e howard-e left a comment

Choose a reason for hiding this comment

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

This is a very welcome addition! Had to do some special handling of this field in #1147 and this should make it a whole lot simpler.

I left small comments inline.

Comment on lines +73 to +74
tpr.minimumAtVersion?.id === minimumAtVersion?.id &&
tpr.exactAtVersion?.id === exactAtVersion?.id
Copy link
Contributor

Choose a reason for hiding this comment

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

To the best of my understanding, only one of these may exist at a time so conditionally checking may be good, with exactAtVersion being the priority. Does that change your thoughts on this comparison?

I suppose it shouldn't matter though, since in the instance of the one (exact or minimum) that doesn't exist, undefined === undefined will be evaluated to true anyways anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think it still makes sense because we are ensuring the Reports are a match and so if one has an undefined minimum then the other would as well. One or the other is going to be a useless check since having only a minimum or a required is probably enforced elsewhere but I think it is a lower cognitive load to read than doing an OR between those two.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough on the cognitive load, this is fine with me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!

const supportedByAutomationResolver = require('./supportedByAutomationResolver');

const AtVersion = {
supportedByAutomation: supportedByAutomationResolver
Copy link
Contributor

@howard-e howard-e Jul 31, 2024

Choose a reason for hiding this comment

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

nit: just for consistency, the other files in /resolvers import the resolver as the expected attribute name, so const supportedByAutomation = require('./supportedByAutomationResolver');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call!

Copy link
Contributor

@howard-e howard-e left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for addressing the feedback

@howard-e howard-e merged commit 3192435 into development Jul 31, 2024
2 checks passed
@howard-e howard-e deleted the improved-supported-by-automation-field branch July 31, 2024 18:21
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