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

Adds types _roff_grid.py #920

Closed
wants to merge 1 commit into from
Closed

Adds types _roff_grid.py #920

wants to merge 1 commit into from

Conversation

janbjorge
Copy link
Contributor

@janbjorge janbjorge commented Oct 10, 2023

Ignore this PR for now, just getting a feel for the system and workflows.

#895

@janbjorge janbjorge self-assigned this Oct 10, 2023
@janbjorge janbjorge changed the title Eagerly added types Eagerly added types _roff_grid.py Oct 10, 2023
@janbjorge janbjorge changed the title Eagerly added types _roff_grid.py Eagerly adds types _roff_grid.py Oct 11, 2023
@@ -50,6 +50,7 @@ examples/*
!examples/*.py
!examples/run_examples.sh
_tmp_*
.dmypy.json
Copy link
Contributor Author

Choose a reason for hiding this comment

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

dmypy is a service that runs mypy as a demon, making mypy checks nearly instant.

@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2023

Codecov Report

Merging #920 (f2dd6c8) into main (71aded5) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #920      +/-   ##
==========================================
- Coverage   80.71%   80.66%   -0.06%     
==========================================
  Files          88       88              
  Lines       13199    13204       +5     
  Branches     2177     2177              
==========================================
- Hits        10654    10651       -3     
- Misses       1833     1847      +14     
+ Partials      712      706       -6     
Files Coverage Δ
src/xtgeo/grid3d/_roff_grid.py 84.31% <100.00%> (+0.52%) ⬆️

... and 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@janbjorge
Copy link
Contributor Author

[.venv] [2023.09.01-py38] [jlov@st-linrgs170 xtgeo]$ dmypy check -- --follow-imports=skip src/xtgeo/grid3d/_roff_grid.py
Success: no issues found in 2 source files

@janbjorge janbjorge changed the title Eagerly adds types _roff_grid.py Adds types _roff_grid.py Oct 11, 2023
@@ -364,7 +373,7 @@ def to_file(self, filelike, roff_format=roffio.Format.BINARY):
roffio.write(filelike, data, roff_format=roff_format)

@staticmethod
def from_file(filelike):
def from_file(filelike: IO) -> RoffGrid:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cant find any closures for this filelike object, this can lead some bugs if the closure only happens when the refcount is zero. I think we should create a task for this for further investigations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe filelike should be Union[str, pathlib.Path, io.BytesIO, io.StringIO]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe filelike should be Union[str, pathlib.Path, io.BytesIO, io.StringIO]

Yeah, i think your right.

@@ -275,7 +280,7 @@ def xtgeo_zcorn(self):
else:
raise ValueError(f"Unknown error {retval} occurred")

def xtgeo_subgrids(self):
def xtgeo_subgrids(self) -> Optional[OrderedDict[str, range]]:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wonder if we could return emtpy dict here insted of none.

@janbjorge janbjorge closed this Oct 30, 2023
@janbjorge janbjorge deleted the add-types-roff-grid branch October 30, 2023 13:19
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.

3 participants