Skip to content

Conversation

@helderjs
Copy link
Collaborator

All Submissions:

  • Have you signed our CLA?
  • Put closes #XXXX in your comment to auto-close the issue that your PR fixes (if there is one).
  • Update docs/release-notes/release-notes.md if your changes should be included in the release notes for the next release.

@helderjs helderjs force-pushed the CLOUDP-175080-project-resources branch from fafed93 to 75dd43c Compare August 22, 2023 14:27
@github-actions
Copy link
Contributor

github-actions bot commented Aug 22, 2023

Copy link
Collaborator

@igor-karpukhin igor-karpukhin left a comment

Choose a reason for hiding this comment

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

Massive work here! Good job! 👍

Comment on lines +236 to +252
if akoMWindow.DayOfWeek != atlasMWindow.DayOfWeek {
return false
}

if akoMWindow.HourOfDay != *atlasMWindow.HourOfDay {
return false
}

if akoMWindow.StartASAP != *atlasMWindow.StartASAP {
return false
}

if akoMWindow.AutoDefer != *atlasMWindow.AutoDeferOnceEnabled {
return false
}

return true
Copy link
Collaborator

@josvazg josvazg Aug 23, 2023

Choose a reason for hiding this comment

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

Why not this instead?

Suggested change
if akoMWindow.DayOfWeek != atlasMWindow.DayOfWeek {
return false
}
if akoMWindow.HourOfDay != *atlasMWindow.HourOfDay {
return false
}
if akoMWindow.StartASAP != *atlasMWindow.StartASAP {
return false
}
if akoMWindow.AutoDefer != *atlasMWindow.AutoDeferOnceEnabled {
return false
}
return true
return akoMWindow.DayOfWeek == atlasMWindow.DayOfWeek &&
akoMWindow.HourOfDay == *atlasMWindow.HourOfDay &&
akoMWindow.StartASAP == *atlasMWindow.StartASAP &&
akoMWindow.AutoDefer == *atlasMWindow.AutoDeferOnceEnabled

For me it is easier to read as it separates the pointer value fixes above from the actual equals check.

Comment on lines +298 to +309
atlasAssignedTeamsInfo := make([]assignedTeamInfo, 0, atlasAssignedTeams.TotalCount)
for _, atlasAssignedTeam := range atlasAssignedTeams.Results {
if atlasAssignedTeam != nil {
atlasAssignedTeamsInfo = append(
atlasAssignedTeamsInfo,
assignedTeamInfo{
ID: atlasAssignedTeam.TeamID,
Roles: atlasAssignedTeam.RoleNames,
},
)
}
}
Copy link
Collaborator

@josvazg josvazg Aug 23, 2023

Choose a reason for hiding this comment

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

nitpick
This seems to be a format conversion. Moving to its own function could made this more readable.

Suggested change
atlasAssignedTeamsInfo := make([]assignedTeamInfo, 0, atlasAssignedTeams.TotalCount)
for _, atlasAssignedTeam := range atlasAssignedTeams.Results {
if atlasAssignedTeam != nil {
atlasAssignedTeamsInfo = append(
atlasAssignedTeamsInfo,
assignedTeamInfo{
ID: atlasAssignedTeam.TeamID,
Roles: atlasAssignedTeam.RoleNames,
},
)
}
}
atlasAssignedTeamsInfo := atlasAssignedTeamsToInfos(atlasAssignedTeams.Results)

Copy link
Collaborator

@josvazg josvazg left a comment

Choose a reason for hiding this comment

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

LGTM

All my comments are non blocking nitpicks

@helderjs helderjs merged commit d3efa56 into main Aug 23, 2023
@helderjs helderjs deleted the CLOUDP-175080-project-resources branch August 23, 2023 09:22
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.

4 participants