Skip to content

feat(apiclient, gate): Add apply_tailoring function #140

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

Merged
merged 4 commits into from
Sep 14, 2022
Merged

Conversation

gegnew
Copy link
Contributor

@gegnew gegnew commented Dec 16, 2021

Fixes #121

I couldn't seem to get CE to actually apply gates to any files, so the only live response I received was:

    {
        "updated": [],
        "inserted": [],
        "deleted": [],
    }

I'm pretty sure that I tested this on the appropriate object, based on the function def in cellengine/app/controllers/gates.server.controller.mjs, but it merits a second set of 👀

Particularly, I used:

    {
        "updated": [{"_id": "updatedGateId", "fcsFileId": "fileId"}],
        "inserted": [],
        "deleted": [{"_id": "deletedGateId", "fcsFileId": "fileId"}],
    }

and a variation of that with multiple objects in the "updated" array.

@gegnew gegnew changed the base branch from master to ge/cattrs-gates December 16, 2021 14:29
@gegnew gegnew force-pushed the ge/121 branch 2 times, most recently from 3d6d026 to b8edb2a Compare December 16, 2021 14:37
@gegnew gegnew requested review from zbjornson and gingo00 December 16, 2021 14:38
@gegnew
Copy link
Contributor Author

gegnew commented Dec 16, 2021

Depends on #136

@gingo00
Copy link

gingo00 commented Dec 16, 2021

Closes #121

I couldn't seem to get CE to actually apply gates to any files, so the only live response I received was:

    {
        "updated": [],
        "inserted": [],
        "deleted": [],
    }

I think CE will apply gates only if the gate provided is different than the currently set gate for this sample for this gate group. By different, I mean a different position (gate model), not different in terms of "_id". So to get this response, I assume that the gate model provided was the same as the one currently applied to the files you tried and therefore, CE did nothing.

@gegnew gegnew force-pushed the ge/121 branch 2 times, most recently from e84a6f2 to 5c13a23 Compare December 20, 2021 11:57
Copy link

@gingo00 gingo00 left a comment

Choose a reason for hiding this comment

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

Tested with different gates to get some inserted/updated/deleted return values and that worked well. See comments about **kwargs

@gegnew gegnew force-pushed the ge/121 branch 2 times, most recently from 897da28 to e8212b6 Compare December 24, 2021 09:46
@gegnew gegnew requested a review from gingo00 December 24, 2021 09:49
@gegnew gegnew force-pushed the ge/cattrs-gates branch 3 times, most recently from 4128181 to 7199a93 Compare January 19, 2022 16:03
@gegnew gegnew force-pushed the ge/121 branch 2 times, most recently from ac6f89a to 4e844c3 Compare January 20, 2022 16:14
@gegnew gegnew force-pushed the ge/cattrs-gates branch from 9da4d58 to e1e4a2d Compare March 7, 2022 05:45
@gegnew gegnew force-pushed the ge/cattrs-gates branch 4 times, most recently from 06548af to d7ba26a Compare March 23, 2022 19:16
gegnew added a commit that referenced this pull request Jun 9, 2022
Instead of requesting gates in a loop, per #140 (comment)
Copy link
Member

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

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

Changes look good, just nits left.

I'll get back to ge/cattrs-gates this week finally.

@zbjornson zbjornson linked an issue Jun 14, 2022 that may be closed by this pull request
gegnew added a commit that referenced this pull request Jun 15, 2022
Instead of requesting gates in a loop, per #140 (comment)
gegnew added a commit that referenced this pull request Jun 15, 2022
Instead of requesting gates in a loop, per #140 (comment)
@gegnew gegnew requested a review from zbjornson June 15, 2022 12:25
zbjornson
zbjornson previously approved these changes Jun 15, 2022
Copy link
Member

@zbjornson zbjornson 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. Will merge once the upstream branch is settled.

@gegnew gegnew force-pushed the ge/cattrs-gates branch from c75429a to 0d418f1 Compare June 16, 2022 15:41
@gegnew gegnew force-pushed the ge/cattrs-gates branch 2 times, most recently from 158ce7f to de34f53 Compare July 5, 2022 07:08
@zbjornson zbjornson force-pushed the ge/cattrs-gates branch 5 times, most recently from c4df360 to 7c59370 Compare September 11, 2022 03:05
Base automatically changed from ge/cattrs-gates to master September 11, 2022 03:06
@zbjornson zbjornson force-pushed the ge/121 branch 2 times, most recently from 021c939 to 33d89be Compare September 11, 2022 03:10
@zbjornson zbjornson dismissed their stale review September 11, 2022 19:40

Working on new issues found

@zbjornson
Copy link
Member

@zbjornson zbjornson merged commit f32a86d into master Sep 14, 2022
@zbjornson zbjornson deleted the ge/121 branch September 14, 2022 05: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.

APIClient.tailor_to behavior is incorrect
4 participants