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

[IO-1461][IO-1820] Code Documentation + exception refactoring + mypy checking #701

Merged
merged 7 commits into from
Oct 25, 2023

Conversation

Nathanjp91
Copy link
Contributor

@Nathanjp91 Nathanjp91 commented Oct 24, 2023

Problem

Some undocumented code and dogfooding issues have accrued

Solution

fix the following:

  • added documentation as well as examples
  • refactored and unified approach to exception handling
  • swapped str/repr locations to conform with mypy typechecking

The List[Exception] Paradigm now only exists for things that parse multiple objects. It should raise on other issues like exceptions from underlying http request, but will attempt to parse all objects before failing.

Changelog

Added documentation and dogfooding refactors to futures code

@linear
Copy link

linear bot commented Oct 24, 2023

IO-1461 Docblock document Query.where

Query.where currently has no doc block documenting it, so IDEs don't hint how it should be used.

Add a docblock with:

  • Params
  • Return
  • Example of use, including MODIFIERS use.

@Nathanjp91 Nathanjp91 changed the title [IO-1461] Code Documentation + exception refactoring + mypy checking [IO-1461][IO-1820] Code Documentation + exception refactoring + mypy checking Oct 24, 2023
@linear
Copy link

linear bot commented Oct 24, 2023

IO-1820 Include params and returns in docblocks in darwin.future

Some functions in darwin-py darwin.future module have descriptions but not params.

Expand to include params and returns, e.g.

From

def my_function(a: int, b: str) -> float:
"""
A function that performs an operation on an int and string and returns a float

Paramters
_________
a  int  The int to use
b  str  The string to use

Returns
_______
float

"""
...

Copy link
Contributor

@owencjones owencjones left a comment

Choose a reason for hiding this comment

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

Yep, a lot of it, but good stuff.

@@ -16,10 +16,13 @@ def remove_dataset(
The client to use to make the request
id : int
The name of the dataset to create
team_slug : str
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch

@@ -43,6 +45,30 @@ def from_exception(cls, exc: Exception) -> "DarwinException":

return instance

@classmethod
Copy link
Contributor

Choose a reason for hiding this comment

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

Could've sworn we already had this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we do in the old code version, I just copied it across and modified a little

@@ -14,7 +14,7 @@ def _collect(self) -> List[Stage]:
raise ValueError("Must specify workflow_id to query stages")
workflow_id: UUID = self.meta_params["workflow_id"]
meta_params = self.meta_params
workflow, exceptions = get_workflow(self.client, str(workflow_id))
workflow = get_workflow(self.client, str(workflow_id))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe need handling of the exceptions here? Consideration point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, I'll add something in. At least a print on failed validations

@owencjones owencjones merged commit 4e7e545 into master Oct 25, 2023
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.

2 participants