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

CS - Add frontend support for new job which takes only a quarter as its parameter #52

Merged
merged 6 commits into from Dec 6, 2022

Conversation

drikdrok
Copy link
Contributor

@drikdrok drikdrok commented Nov 30, 2022

@drikdrok drikdrok changed the title Add frontend support for new job which takes only a quarter as its parameter CS - Add frontend support for new job which takes only a quarter as its parameter Nov 30, 2022
@drikdrok drikdrok added the f22-5pm-1 f22-5pm-1 label Nov 30, 2022
@pconrad pconrad added the FIXME: Peer CR Please get a peer code review label Nov 30, 2022
@tallyhawley tallyhawley force-pushed the ChristianSupportForNewQuarterJob branch from 8338cc7 to a6437ee Compare November 30, 2022 08:39
@tallyhawley tallyhawley self-requested a review November 30, 2022 08:53
tallyhawley
tallyhawley previously approved these changes Nov 30, 2022
Copy link
Contributor

@tallyhawley tallyhawley left a comment

Choose a reason for hiding this comment

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

lgtm

@liamjet liamjet added STAFF: ready for review green on CI, has peer CR, no other obvious issues and removed FIXME: Peer CR Please get a peer code review labels Nov 30, 2022
Copy link
Contributor

@pconrad pconrad left a comment

Choose a reason for hiding this comment

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

LGTM.. will test on qa

@pconrad pconrad temporarily deployed to f22-5pm-1-courses December 1, 2022 23:36 Inactive
@pconrad pconrad temporarily deployed to f22-5pm-1-courses December 4, 2022 23:33 Inactive
@pconrad pconrad added FIXME: Changes Requested Please address the changes specified in the code review and removed STAFF: ready for review green on CI, has peer CR, no other obvious issues labels Dec 4, 2022
Copy link
Contributor

@pconrad pconrad left a comment

Choose a reason for hiding this comment

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

Please consider making the scope of Stryker exclusions as small as possible, as illustrated in this PR: #57


const quarters = quarterRange(startQtr, endQtr);

// Stryker disable all : not sure how to test/mock local storage
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be followed by
// Stryker enable all
after the line of code that does local storage.

callback({quarter});
};

// Stryker disable all : Stryker is testing by changing the padding to 0. But this is simply a visual optimization as it makes it look better
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above... it's ok to invoke this for the part of the code that needs Stryker disabled... but you need to confine this to the smallest possible scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

In #57 I suggest what this might look like.

…terJob-pc

pc - make stryker exclusions as small as possible
lgtm
Copy link
Contributor

@pconrad pconrad left a comment

Choose a reason for hiding this comment

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

LGTM

@pconrad pconrad added 10 10 pts and removed FIXME: Changes Requested Please address the changes specified in the code review labels Dec 6, 2022
@pconrad pconrad merged commit 30e296c into main Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10 10 pts f22-5pm-1 f22-5pm-1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add frontend support for new job which takes only a quarter as its parameter
4 participants