AI-SDC / ACRO

Tools for the Automatic Checking of Research Outputs. These are the tools for researchers to use as drop-in replacements for commands that produce outputs in Stata Python and R
MIT License
15 stars 2 forks source link

140 move stata handling to acro folder and improve formatting on screen #142

Closed jim-smith closed 1 year ago

codecov[bot] commented 1 year ago

Codecov Report

Merging #142 (137e4ae) into main (511c90b) will increase coverage by 2.68%. The diff coverage is 100.00%.

@@             Coverage Diff             @@
##             main      #142      +/-   ##
===========================================
+ Coverage   97.31%   100.00%   +2.68%     
===========================================
  Files           6         7       +1     
  Lines         708       781      +73     
===========================================
+ Hits          689       781      +92     
+ Misses         19         0      -19     
Files Changed Coverage Δ
acro/acro_stata_parser.py 100.00% <100.00%> (ø)
acro/record.py 100.00% <100.00%> (+1.47%) :arrow_up:
acro/stata_config.py 100.00% <100.00%> (ø)
acro/utils.py 100.00% <100.00%> (+1.76%) :arrow_up:

... and 1 file with indirect coverage changes

rpreen commented 1 year ago

mypy is picking up a number of type errors. See pre-commit output

also, the static analysis is not happy about: Use of possibly insecure function - consider using safer ast.literal_eval.
on https://github.com/AI-SDC/ACRO/blob/9469f7b443d0cb8063314d33fba5bcdeef8af2c0/acro/acro_stata_parser.py#L35

jim-smith commented 1 year ago

Yes mypy is - but that is because of two things: 1: its really odd about dictionaries 2: it is only doing static analysis

Professor Jim Smith Department of Computer Science and Creative Technologies University of the West of England Bristol BS16 1QY, UK t: +44 117 3287417 w: www.fet.uwe.ac.uk/~jsmith Orcidhttps://orcid.org/0000-0001-7908-1859 :

See my availability @.***/6ff8c362328448d49c717917ae5cd3c66650096615907872250/calendar.html>:

Find out about my new UKRI-funded project here: https://dareuk.org.uk/driver-project-sacro/

On 15 Aug 2023, at 13:34, Richard Preen @.***> wrote:

mypy is picking up a number of type errors. See pre-commit outputhttps://results.pre-commit.ci/run/github/534172863/1692102464.uwpCstbnRDaCNLPpVN8kTw

also, the static analysis is not happyhttps://app.codacy.com/gh/AI-SDC/ACRO/pullRequest?prid=12415414 about: Use of possibly insecure function - consider using safer ast.literal_eval. on https://github.com/AI-SDC/ACRO/blob/9469f7b443d0cb8063314d33fba5bcdeef8af2c0/acro/acro_stata_parser.py#L35

— Reply to this email directly, view it on GitHubhttps://github.com/AI-SDC/ACRO/pull/142#issuecomment-1678856873, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACZSEI3HZAO2Q4V55DGWUZTXVNUF5ANCNFSM6AAAAAA3Q6XQDM. You are receiving this because you authored the thread.Message ID: @.***>

jim-smith commented 1 year ago

Added lots more tests and a few pragma:no cover around try...except cases

patch coverage is reporting la fraction of a percent low because I test the function that do stata subsetting separately rather than adding extra tests to do the same thing via an acro table command. Will add a test to box those off later, but otherwise / notwithstanding, good to review I think

jim-smith commented 1 year ago

@rpreen Think I have addressed everything - this gets a clean bill of health and improve the overall coverage of acro to 100% (with one or two lines pragma'd out for valid reasons)

BUT I spent ages working on testing the aggregation functions only to realise I'd uncovered a bug /assumption in the code that does dominate/threshold checking - separate issue raised