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 SafeBase Module #29

Merged
merged 9 commits into from
Nov 15, 2024
Merged

Add SafeBase Module #29

merged 9 commits into from
Nov 15, 2024

Conversation

0x0f0f0f
Copy link
Owner

  • add safe base module
  • Add SafeBase
  • add more tests

@0x0f0f0f
Copy link
Owner Author

@iamed2 would you have a moment to take a look at this?

src/SafeBase.jl Outdated Show resolved Hide resolved
src/SafeBase.jl Outdated
This function can optionally throw if an exception is encountered.
Otherwise, a silent failure is indicated by returning `nothing`.
"""
function safe_eval(m::Module, expr)::Result{Any,EvalError}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What types can this accept? Why such a different implementation here?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Should be Any.

src/SafeBase.jl Outdated Show resolved Hide resolved
@iamed2
Copy link
Collaborator

iamed2 commented Aug 27, 2024

I can look more this weekend, in the meantime can you make src/SafeBase.jl more BlueStyle? And add signatures to the new method docstrings that don't contain them.

@0x0f0f0f
Copy link
Owner Author

0x0f0f0f commented Sep 2, 2024

@iamed2 addressed the comments. Thanks!

@0x0f0f0f 0x0f0f0f marked this pull request as ready for review September 23, 2024 13:34
@0x0f0f0f
Copy link
Owner Author

Can I go ahead and merge? @iamed2

@0x0f0f0f 0x0f0f0f merged commit ef5a546 into master Nov 15, 2024
3 checks passed
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