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

Update README.md to fix typo #91

Merged
merged 1 commit into from
Mar 29, 2023
Merged

Conversation

kdheepak
Copy link
Contributor

This is the current README's example:

julia> @macroexpand @infiltrate cond=true
quote
    if cond = true
        (Infiltrator.start_prompt)(Main, $(Expr(:locals)), "REPL[16]", 1)
    end
end

But I think this is what it should be:

julia> @macroexpand @infiltrate cond==true
quote
    if cond == true
        (Infiltrator.start_prompt)(Main, $(Expr(:locals)), "REPL[17]", 1)
    end
end

@pfitzseb
Copy link
Member

This is just a super confusing docstring. The = true part was meant as a default argument, not a comparison (ref).

Might be best to remove that altogether...

@kdheepak
Copy link
Contributor Author

How about this change instead?

@pfitzseb
Copy link
Member

Makes sense to me. Wanna reflect the same change in the docstring as well?

@codecov-commenter
Copy link

codecov-commenter commented Mar 29, 2023

Codecov Report

Merging #91 (2297b63) into master (130b75e) will not change coverage.
The diff coverage is n/a.

❗ Current head 2297b63 differs from pull request most recent head 05e53ba. Consider uploading reports for the commit 05e53ba to get more accurate results

@@           Coverage Diff           @@
##           master      #91   +/-   ##
=======================================
  Coverage   74.15%   74.15%           
=======================================
  Files           1        1           
  Lines         383      383           
=======================================
  Hits          284      284           
  Misses         99       99           
Impacted Files Coverage Δ
src/Infiltrator.jl 74.15% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@kdheepak
Copy link
Contributor Author

I think it might be better in the docstring to use a condition::Bool to indicate that the macro takes an optional argument. I'm inclined to make the same change in the README as well actually:

@infiltrate
@infiltrate condition::Bool

Thoughts?

@pfitzseb
Copy link
Member

Makes sense to me.

@kdheepak kdheepak force-pushed the patch-1 branch 2 times, most recently from 5b4bf23 to 23fb8fa Compare March 29, 2023 19:40
This commit also fixes some whitespace changes in the README
@kdheepak
Copy link
Contributor Author

kdheepak commented Mar 29, 2023

I did a rebase on top of the latest README in the master branch, and also fixed some whitespace changes. And I squashed all the commits into one commit for making it easier to read.

Copy link
Member

@pfitzseb pfitzseb left a comment

Choose a reason for hiding this comment

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

Thanks!

@pfitzseb pfitzseb merged commit e0cc824 into JuliaDebug:master Mar 29, 2023
@kdheepak kdheepak deleted the patch-1 branch March 29, 2023 19:48
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