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

Add support for SmartAsserts.jl #152

Merged
merged 11 commits into from
Jun 20, 2023
Merged

Add support for SmartAsserts.jl #152

merged 11 commits into from
Jun 20, 2023

Conversation

Zentrik
Copy link
Contributor

@Zentrik Zentrik commented Apr 16, 2023

It should be easy to add any other similar package such as ArgChecks.jl

@mauro3
Copy link
Owner

mauro3 commented Apr 30, 2023

Thanks!

Comments:

  • adding this feature would be good
  • shouldn't ArgCheck.jl be implemented as on https://github.com/MrVPlusOne/SmartAsserts.jl it states that that package is preferred?
  • The tests don't actually test the functionality or am I missing something?

x-ref: #120

@Zentrik
Copy link
Contributor Author

Zentrik commented May 2, 2023

I decided not to use ArgCheck as it's ability to compile out asserts is not supported on new versions of julia, OptionalArgChecks.jl is broken on >= 1.5.
I would have added ArgCheck as well in this pr but I couldn't figure out a way to add the test without just copy and pasting them, which I didn't want to do as otherwise you would have 3 copies of near identical tests. If you don't care about this I can add ArgCheck as well.

I've updated the tests, thanks for spotting that.

The main problem now is I didn't notice SmartAssert has set its compat to julia 1.5, I'll see if I can get that changed.

@codecov
Copy link

codecov bot commented Jun 18, 2023

Codecov Report

Merging #152 (ed27133) into master (e55b025) will increase coverage by 0.32%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #152      +/-   ##
==========================================
+ Coverage   93.42%   93.75%   +0.32%     
==========================================
  Files           1        1              
  Lines         304      304              
==========================================
+ Hits          284      285       +1     
+ Misses         20       19       -1     
Impacted Files Coverage Δ
src/Parameters.jl 93.75% <100.00%> (+0.32%) ⬆️

@Zentrik
Copy link
Contributor Author

Zentrik commented Jun 18, 2023

This should be ready to be merged in now.

Copy link
Owner

@mauro3 mauro3 left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests! There are still a few bits to do. Also, let's move the compat for Julia to 1.5.

Manifest.toml Outdated Show resolved Hide resolved
Project.toml Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
@mauro3
Copy link
Owner

mauro3 commented Jun 20, 2023

Sorry, one more thing: now we just need some docs! Nothing fancy, just add to where @assert is described a note that SmartAsserts.jl is also supported.

@mauro3 mauro3 merged commit 501b47b into mauro3:master Jun 20, 2023
@mauro3
Copy link
Owner

mauro3 commented Jun 20, 2023

Thanks so much for your help!

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