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

Updates in the data sources api reference #1795

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

luisaFelixSalles
Copy link
Collaborator

Updates the DataSources API reference

  • Type hints in all the functions ans properties of the class
  • More examples and explanations

@luisaFelixSalles luisaFelixSalles added the documentation Improvements or additions to documentation label Oct 7, 2024
@luisaFelixSalles luisaFelixSalles self-assigned this Oct 7, 2024
@luisaFelixSalles luisaFelixSalles mentioned this pull request Sep 6, 2024
2 tasks
self,
result_path: Optional[Union[str, os.PathLike]] = None,
data_sources: Optional[dpf.DataSources] = None,
server: Optional[server_types.DpfServer] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not server_types.DpfServer.
Either the BaseServer class, or a Union of LegacyGrpcServer, GrpcServer, and InProcessServer. I think the Union is best.

def __init__(
self,
result_path: Optional[Union[str, os.PathLike]] = None,
data_sources: Optional[dpf.DataSources] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a dpf.DataSources.
A Union of either int for a memory address or ansys.grpc.dpf.data_sources_pb2.DataSources as in the original signature.

self, path: Union[str, os.PathLike], domain_id: int, key: Union[str, None] = None
):
"""Add a result file path by domain.
self, path: Union[str, os.PathLike], domain_id: int, key: Optional[str] = None
Copy link
Contributor

@PProfizi PProfizi Oct 7, 2024

Choose a reason for hiding this comment

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

Suggested change
self, path: Union[str, os.PathLike], domain_id: int, key: Optional[str] = None
self, path: Union[str, os.PathLike], domain_id: int, key: Optional[Union[str, None]] = None

I think here None has to be listed as a valid type for the key argument.

Copy link
Collaborator Author

@luisaFelixSalles luisaFelixSalles Oct 7, 2024

Choose a reason for hiding this comment

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

I think when we use optional is exactly to omit the "None" in a union :

image

https://peps.python.org/pep-0484/#annotating-generator-functions-and-coroutines

Copy link

codecov bot commented Oct 7, 2024

Codecov Report

Attention: Patch coverage is 78.57143% with 3 lines in your changes missing coverage. Please review.

Project coverage is 88.11%. Comparing base (a314cd0) to head (da2e685).
Report is 35 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1795      +/-   ##
==========================================
+ Coverage   85.64%   88.11%   +2.46%     
==========================================
  Files          83       83              
  Lines        9937     9956      +19     
==========================================
+ Hits         8511     8773     +262     
+ Misses       1426     1183     -243     

@@ -44,9 +46,16 @@
from ansys.dpf.core.check_version import version_requires
from ansys.dpf.core import errors

if TYPE_CHECKING:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if TYPE_CHECKING:
if TYPE_CHECKING: # pragma: no cover

- If you added an upstream result file, it is not listed in the main ``DataSources`` object. You have to
check directly in the ``DataSources`` object created to define the upstream data.

Examples
Copy link
Contributor

Choose a reason for hiding this comment

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

@luisaFelixSalles maybe the second Examples section title is the source of error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants