Skip to content

Create codeql-analysis.yml #1127

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

Merged
merged 2 commits into from
Sep 22, 2022
Merged

Create codeql-analysis.yml #1127

merged 2 commits into from
Sep 22, 2022

Conversation

joshmoore
Copy link
Member

see:

[Description of PR]

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@joshmoore
Copy link
Member Author

Primary TODO is convert:

from tempfile import mktemp

def write_results(results):
    filename = mktemp()
    with open(filename, "w+") as f:
        f.write(results)
    print("Results written to", filename)

to

from tempfile import NamedTemporaryFile

def write_results(results):
    with NamedTemporaryFile(mode="w+", delete=False) as f:
        f.write(results)
    print("Results written to", f.name)

@DimitriPapadopoulos
Copy link
Contributor

DimitriPapadopoulos commented Sep 9, 2022

CodeQL fails on multiple occurrences of the issue documented in #906.

Should we shut up this warning, if possible at all?

@joshmoore
Copy link
Member Author

Have you seen how to do that configuration? If not, perhaps it's time to tackle #906.

@DimitriPapadopoulos
Copy link
Contributor

DimitriPapadopoulos commented Sep 9, 2022

See Dismissing alerts:

There are two ways of closing an alert. You can fix the problem in the code, or you can dismiss the alert.

Dismissing an alert is a way of closing an alert that you don't think needs to be fixed. For example, an error in code that's used only for testing, or when the effort of fixing the error is greater than the potential benefit of improving the code. You can dismiss alerts from code scanning annotations in code, or from the summary list within the Security tab.

It looks like we cannot easily dismiss the alerts from a configuration file or with a comment in the line of code that generates the alert.

Adds zarr.tests.util.mktemp which can be used from all
tests. The NamedTemporaryFile is immediately closed and
only the path returned.
@codecov
Copy link

codecov bot commented Sep 16, 2022

Codecov Report

Merging #1127 (a5c65e3) into main (7ae1de7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1127   +/-   ##
=======================================
  Coverage   99.95%   99.95%           
=======================================
  Files          36       36           
  Lines       14112    14117    +5     
=======================================
+ Hits        14105    14110    +5     
  Misses          7        7           
Impacted Files Coverage Δ
zarr/tests/test_core.py 100.00% <100.00%> (ø)
zarr/tests/test_creation.py 100.00% <100.00%> (ø)
zarr/tests/test_hierarchy.py 100.00% <100.00%> (ø)
zarr/tests/test_storage.py 100.00% <100.00%> (ø)
zarr/tests/test_storage_v3.py 100.00% <100.00%> (ø)
zarr/tests/util.py 100.00% <100.00%> (ø)

@joshmoore
Copy link
Member Author

@DimitriPapadopoulos, that seems to have done it.

@joshmoore
Copy link
Member Author

Green after a restart. Let's merge and see if it helps or hinders. (We can always disable)

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