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

Fix experiment table changes collection bug #2598

Merged
merged 5 commits into from
Oct 17, 2022
Merged

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented Oct 15, 2022

Noticed a couple of issues with the code whilst staring at it after reading #2569 (comment).

@mattseddon mattseddon added the bug Something isn't working label Oct 15, 2022
@mattseddon mattseddon self-assigned this Oct 15, 2022
let workspace
let currentCommit

for (const [key, value] of Object.entries(data)) {
Copy link
Member Author

@mattseddon mattseddon Oct 15, 2022

Choose a reason for hiding this comment

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

This would not return the correct data now there are > 2 entries in the exp show data. It would actually return the oldest commit

@@ -109,7 +110,7 @@ const collectChange = (
commitData: ExperimentFields,
ancestors: string[] = []
) => {
if (typeof value === 'object') {
if (!Array.isArray(value) && typeof value === 'object') {
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] If we get to an array we want to compare the array, not try to walk it.

@mattseddon mattseddon marked this pull request as ready for review October 16, 2022 22:58
@codeclimate
Copy link

codeclimate bot commented Oct 17, 2022

Code Climate has analyzed commit 3009a26 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.8%.

View more on Code Climate.

@mattseddon mattseddon merged commit 7c9855e into main Oct 17, 2022
@mattseddon mattseddon deleted the fix-collect-bug branch October 17, 2022 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants