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

Rewrite, postpone and pause #308

Merged
merged 4 commits into from
May 23, 2019
Merged

Rewrite, postpone and pause #308

merged 4 commits into from
May 23, 2019

Conversation

mlhetland
Copy link
Contributor

This PR has three minor modifications to entr (cf. #298):

  • By default, entr will now execute f() initially, unless the postpone keyword is used;
  • entr sleeps for the amount given by pause between being triggered and executing f(), to handle clusters of modifications, such as text editor saves; and
  • Rewrites to a given file/file name are now accepted as modifications; rewrites from the file name cause an error, as the file with the supplied name no longer exists.

The first and last are in line with the entr command line tool. The second is an added convenience, to avoid redundant execution when (e.g.) generating output based on saved files, etc.

The modifications are independent – no need to accept them all.

(Note: No new tests have been added for these features.)

mlhetland added 3 commits May 13, 2019 13:19
The function is now executed initially as well, unless postpone=true.
Now entr will trigger also if a file is overwritten by the renaming of another.
entr will now pause between being triggered and actually executing, to handle small clusters of modifications to a single file.
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.

I like all of these changes! Thanks for keeping them in separate PRs, when merged let's keep them that way.

Feel free to add any tests that you want. If some behaviors are difficult to test, I'll leave it up to your judgement to decide where to draw the line; let me know when you want this merged and I'll do it.

src/Revise.jl Outdated Show resolved Hide resolved
Co-Authored-By: Tim Holy <[email protected]>
@codecov
Copy link

codecov bot commented May 14, 2019

Codecov Report

Merging #308 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #308      +/-   ##
==========================================
+ Coverage   75.63%   75.66%   +0.02%     
==========================================
  Files          11       11              
  Lines        1133     1134       +1     
==========================================
+ Hits          857      858       +1     
  Misses        276      276
Impacted Files Coverage Δ
src/Revise.jl 71.25% <100%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e14af8...69465c3. Read the comment docs.

3 similar comments
@codecov
Copy link

codecov bot commented May 14, 2019

Codecov Report

Merging #308 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #308      +/-   ##
==========================================
+ Coverage   75.63%   75.66%   +0.02%     
==========================================
  Files          11       11              
  Lines        1133     1134       +1     
==========================================
+ Hits          857      858       +1     
  Misses        276      276
Impacted Files Coverage Δ
src/Revise.jl 71.25% <100%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e14af8...69465c3. Read the comment docs.

@codecov
Copy link

codecov bot commented May 14, 2019

Codecov Report

Merging #308 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #308      +/-   ##
==========================================
+ Coverage   75.63%   75.66%   +0.02%     
==========================================
  Files          11       11              
  Lines        1133     1134       +1     
==========================================
+ Hits          857      858       +1     
  Misses        276      276
Impacted Files Coverage Δ
src/Revise.jl 71.25% <100%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e14af8...69465c3. Read the comment docs.

@codecov
Copy link

codecov bot commented May 14, 2019

Codecov Report

Merging #308 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #308      +/-   ##
==========================================
+ Coverage   75.63%   75.66%   +0.02%     
==========================================
  Files          11       11              
  Lines        1133     1134       +1     
==========================================
+ Hits          857      858       +1     
  Misses        276      276
Impacted Files Coverage Δ
src/Revise.jl 71.25% <100%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e14af8...69465c3. Read the comment docs.

@mlhetland
Copy link
Contributor Author

Feel free to add any tests that you want. If some behaviors are difficult to test, I'll leave it up to your judgement to decide where to draw the line; let me know when you want this merged and I'll do it.

There should probably be a couple of added tests in the "entr" test set. I've got it on my todo list, but if anyone is waiting for this fix/functionality, feel free to add/suggest some – not sure when I'll get to it.

@timholy
Copy link
Owner

timholy commented May 23, 2019

If you think it will be a long time, like I said I'm willing to merge this now and risk breakage later. I kind of think of entr as "a community supported extra" to Revise, so I'm fine if the community is willing to risk breakage by going without thorough tests.

Just let me know what you want me to do.

@mlhetland
Copy link
Contributor Author

I'm fine with that. I guess if there's a bug that needs fixing, we could add a test provoking that bug, anyway :-)

@timholy timholy merged commit 580e991 into timholy:master May 23, 2019
@timholy
Copy link
Owner

timholy commented May 23, 2019

Technically this is breaking (previously we effectively had postpone=true), but I think of the old behavior as a bug, so I'll release this as Revise 2.2 rather than Revise 3.0.

@mlhetland
Copy link
Contributor Author

Yes, I thought about that, too (specifically the postpone thing) – but I guess breaking changes don't depend on actual behavior, but documented behavior (if you're going with semantic versioning), so I guess the question here is how clear an expectation one might have, from the documentation, that there would be no call until the file changes. It states that it'll be called whenever the file is updated, of course, so I guess it would be reasonable to infer that it wouldn't be called until the file is updated post calling entr – but given the rather high-level description, it's not something I'd have relied on. (Then again, to find the specific behavior, I probably would have consulted the source, so … :-})

But despite being (mildly) breaking, the fix (postpone=true) is very easy to add. And, as you say, given the naming (from the command-line tool), it's not unreasonable to see this as a bug.

That reminds me: I didn't modify any documentation (beyond the docstring itself). If it's mentioned elsewhere, that should probably also be changed?

@timholy
Copy link
Owner

timholy commented May 23, 2019

Improvement to the documentation would be most welcome. Note #311, which I think is due to JuliaInterpreter not working on nightly. (I guess fixing that should be my next priority.)

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