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

feat: [VRD-818] Implement BuildScan class #3757

Merged
merged 27 commits into from Apr 25, 2023
Merged

feat: [VRD-818] Implement BuildScan class #3757

merged 27 commits into from Apr 25, 2023

Conversation

liuverta
Copy link
Contributor

Impact and Context

Now we have a user-facing class to represent the results of a model build scan!

Risks and Area of Effect

None; entirely new functionality, covered by tests.

Testing

  • Unit test
  • Deployed to dev env
  • Other (explain)

In Python 3.10:

% pytest unit_tests/deployment/test_build_scan.py 
==================================================== test session starts ====================================================
platform darwin -- Python 3.10.6, pytest-7.2.1, pluggy-1.0.0
rootdir: /Users/miliu/Documents/modeldb/client/verta/tests, configfile: pytest.ini
plugins: forked-1.4.0, xdist-3.1.0, typeguard-2.13.3, hypothesis-6.67.1
collected 1 item                                                                                                            

unit_tests/deployment/test_build_scan.py .                                                                            [100%]

===================================================== 1 passed in 0.43s =====================================================

Reverting

  • Contains Migration - Do Not Revert

Revert this PR.

Comment on lines +8 to +11
{% if objname.endswith("Enum") -%}
:undoc-members:
:member-order: bysource
{% endif -%}
Copy link
Contributor Author

@liuverta liuverta Apr 20, 2023

Choose a reason for hiding this comment

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

Enum variants are technically class members, and there isn't a clean way to attach meaningful docstrings to them, so we have to clumsily tell our documentation, "If a class's name ends with Enum, go ahead and render its undocumented members" as a bit of a hack.

Copy link
Contributor

Choose a reason for hiding this comment

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

This bit gives you the OPTION = 'option' style listing of all the possible options at the end of the doc string I assume?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes


assert build.get_scan().progress == "scanned"

"""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2023-04-20 at 4 03 41 PM


assert build.get_scan().status == "safe"

"""
Copy link
Contributor Author

@liuverta liuverta Apr 20, 2023

Choose a reason for hiding this comment

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

Screen Shot 2023-04-25 at 9 47 26 AM


(build_scan.progress == "scanned") and (build_scan.get_status() == "safe")

"""
Copy link
Contributor Author

@liuverta liuverta Apr 20, 2023

Choose a reason for hiding this comment

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

Screen Shot 2023-04-25 at 9 47 14 AM

Copy link
Contributor Author

@liuverta liuverta Apr 20, 2023

Choose a reason for hiding this comment

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

Note Build.get_scan() isn't hyperlinked, because it isn't implemented until #3758.

Copy link
Contributor

@ewagner-verta ewagner-verta left a comment

Choose a reason for hiding this comment

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

The only minor concern I have is the use of the term "status" , as it could be a bit confusing and potentially be conflated with "progress". (Checking the progress of a scan could arguably mean the same thing as checking the status of a scan). I wonder if something like ScanResultEnum / Buildscan.result might make it more explicit that this object represents the outcome/result of a completed scan.

Base automatically changed from liu/build-datetime to main April 25, 2023 16:30
@liuverta
Copy link
Contributor Author

I've addressed @ewagner-verta's feedback by replacing any references to a build scan's "status" with "result".

Tests still pass, screenshots of documentation have been updated, and a cmd+F of this PR's diff for "status" confirms that the word is no longer being used in the user-facing client API.

@liuverta liuverta merged commit ab71768 into main Apr 25, 2023
6 of 7 checks passed
@liuverta liuverta deleted the liu/build-scan branch April 25, 2023 16:52
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.

None yet

3 participants