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

Adding more general function with better name #893

Merged
merged 12 commits into from
Sep 2, 2022
Merged

Conversation

mojtaba-komeili
Copy link
Contributor

@mojtaba-komeili mojtaba-komeili commented Aug 30, 2022

Description

We have the direct_soft_block_mturk_workers function which is great to assign any qualification to workers, including the soft blocking qualification. The convenience of this often tempts people to use it for any general qualification (example). This sort of use causes confusion and may end up with logical bugs. This patch renames direct_soft_block_mturk_workers to direct_assign_qual_mturk_workers (with minimal changes). To keep the backward compatibility, we still have direct_soft_block_mturk_workers; but it is just a wrapper around the call to direct_assign_qual_mturk_workers.

Test

Used it with assigning allow list qualification in this PR, and it worked well. Checking the final results proved that it assigned the qualification correctly.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 30, 2022
mojtaba-komeili and others added 2 commits August 30, 2022 17:18
While retrying this step, realized that we need `pip install pre-commit` if it is not already installed.
@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2022

Codecov Report

Merging #893 (66294cd) into main (51a2b1d) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #893      +/-   ##
==========================================
- Coverage   64.63%   64.62%   -0.02%     
==========================================
  Files         108      108              
  Lines        9320     9320              
==========================================
- Hits         6024     6023       -1     
- Misses       3296     3297       +1     
Impacted Files Coverage Δ
mephisto/abstractions/databases/local_database.py 89.80% <100.00%> (ø)
...isto/abstractions/providers/mock/mock_datastore.py 53.92% <100.00%> (ø)
...to/abstractions/providers/mturk/mturk_datastore.py 61.48% <100.00%> (ø)
mephisto/data_model/unit.py 77.59% <0.00%> (-0.55%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mojtaba-komeili
Copy link
Contributor Author

@JackUrb not sure what they cypress one is about. Could you give me a hint please?

@JackUrb
Copy link
Contributor

JackUrb commented Aug 31, 2022

Hm something strange is happening with the build flow. I'll see if I can reproduce on the other dependency branch I'm working on.

Copy link
Contributor

@JackUrb JackUrb left a comment

Choose a reason for hiding this comment

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

Quality change!

As far as the testing goes, it seems that cypress tests are failing on main at the moment. Need to debug the why.

@mojtaba-komeili
Copy link
Contributor Author

Quality change!

As far as the testing goes, it seems that cypress tests are failing on main at the moment. Need to debug the why.

I wait for the new changes you had to land and retry after rebasing with that.

@mojtaba-komeili mojtaba-komeili merged commit 48c0c23 into main Sep 2, 2022
@mojtaba-komeili mojtaba-komeili deleted the assign-qual branch October 5, 2022 03:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants