Skip to content

Tapir Safe Mode#259

Merged
gdalle merged 5 commits intoJuliaDiff:mainfrom
willtebbutt:wct/tapir-safe-mode
May 14, 2024
Merged

Tapir Safe Mode#259
gdalle merged 5 commits intoJuliaDiff:mainfrom
willtebbutt:wct/tapir-safe-mode

Conversation

@willtebbutt
Copy link
Copy Markdown
Member

Follow up from recent ADTypes PR -- this PR ensures that if someone asks for safe mode in when constructing AutoTapir, they get safe mode.

@willtebbutt
Copy link
Copy Markdown
Member Author

Test failures because version 1.2 of ADTypes is not yet available.

@willtebbutt willtebbutt reopened this May 14, 2024
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.13%. Comparing base (f66c0bc) to head (d9cbb3b).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #259   +/-   ##
=======================================
  Coverage   95.13%   95.13%           
=======================================
  Files          74       74           
  Lines        3929     3929           
=======================================
  Hits         3738     3738           
  Misses        191      191           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gdalle
Copy link
Copy Markdown
Member

gdalle commented May 14, 2024

The formatting check failed. Also you are polluting my test suite with @info messages, is there any way to get rid of them? Perhaps test the unsafe mode here, and both modes on your end?

@willtebbutt
Copy link
Copy Markdown
Member Author

The formatting check failed.

Hmm yes, that does appear to be what's happening. Have you considered deploying ReviewDog e.g. as done in AbstractGPs in order to make any formatting issues + fixes appear in the PR?

Also you are polluting my test suite with @info messages, is there any way to get rid of them?

Hahaha, yes, I am doing that at present.

Perhaps test the unsafe mode here, and both modes on your end?

Yeah, that's probably the right thing to do. Will fix these problems now.

@gdalle
Copy link
Copy Markdown
Member

gdalle commented May 14, 2024

Have you considered deploying ReviewDog e.g. as done in AbstractGPs in order to make any formatting issues + fixes appear in the PR?

From what I've seen, these tools can be extremely noisy and make tons of comments on a single commit, so I like having that check as part of my test suite. Especially when I work on rather ambitious PRs, which is often the case in DI

@willtebbutt
Copy link
Copy Markdown
Member Author

From what I've seen, these tools can be extremely noisy and make tons of comments on a single commit, so I like having that check as part of my test suite. Especially when I work on rather ambitious PRs, which is often the case in DI

That's completely reasonable.

@willtebbutt
Copy link
Copy Markdown
Member Author

Another bug on my end because testing whether something prints is hard. Will fix and then re-run the tests.

@gdalle
Copy link
Copy Markdown
Member

gdalle commented May 14, 2024

The printing is not a problem for me, it's just a mild annoyance. Since the tests pass, can I merge and release this?

@willtebbutt
Copy link
Copy Markdown
Member Author

Yes, please do. The next patch version of Tapir will fix the printing issue anyway.

@gdalle gdalle merged commit 8aa5bb4 into JuliaDiff:main May 14, 2024
@willtebbutt willtebbutt deleted the wct/tapir-safe-mode branch May 14, 2024 17:44
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