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

Fix #470 #471

Closed
wants to merge 2 commits into from
Closed

Fix #470 #471

wants to merge 2 commits into from

Conversation

samanklesaria
Copy link

@samanklesaria samanklesaria commented Apr 30, 2020

Line 822 of Revise.jl calls FileWatching.watch_folder. It then calls ret.changed on the result. But FileWatching.watch_folder returns a pair:

julia> FileWatching.watch_folder("src")
"extra.jl" => FileWatching.FileEvent(false, true, false)

As the documentation says:

The returned value is an pair where the first field is the name of the changed file (if available) and the second field is an object with boolean fields changed, renamed, and timedout, giving the event.

Copy link
Owner

@timholy timholy left a comment

Choose a reason for hiding this comment

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

Thank for the PR!

The docstring of entr doesn't claim to support directories, though obviously the code has been prepared to do so. It looks like none of the tests check this case, though.

I'm good with this change, though a directory test would make it even better. CC @mlhetland, since he's kind of the de facto expert on entr.

src/Revise.jl Outdated
if is_file_dir
ret = watch_folder(file, 1)[2]
else
ret = watch_File(file, 1)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
ret = watch_File(file, 1)
ret = watch_file(file, 1)

@mlhetland
Copy link
Contributor

CC @mlhetland, since he's kind of the de facto expert on entr.

Heh, more a fan than an expert, but thanks :-) Seems like a good idea to me. (The original entr also has the ability to watch directories. It requires a specific flag, -d, but I see no need for that in our case, really, as we'll be pretty explicit about supplying the directory name, anyway?)

@samanklesaria
Copy link
Author

I updated the docstrings, and I'm happy to add a test too. Is there any way to run a subset of tests in the suite, rather than of all of them (as in Pkg.test)?

@timholy
Copy link
Owner

timholy commented May 11, 2020

Sorry for the delay, I have been very distracted lately. Yes, you can run a subset of tests:

  • julia 1.3 or higher: Pkg.test("Revise", test_args=["Method deletion"]) (you can add more than one testset)
  • from the shell prompt, if you have all test dependencies in your environment: julia runtests.jl "Method deletion"
  • from an interactive session: push!(ARGS, "Method deletion"); include("runtests.jl")

@ffevotte
Copy link
Collaborator

ffevotte commented Jun 7, 2020

Just to tidy things up and let everyone interested know:

@timholy timholy closed this Jun 7, 2020
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.

4 participants