Skip to content
This repository has been archived by the owner on Jan 18, 2025. It is now read-only.

Big rewrite of the application #30

Merged
merged 16 commits into from
Jan 31, 2016
Merged

Big rewrite of the application #30

merged 16 commits into from
Jan 31, 2016

Conversation

LukasKalbertodt
Copy link
Contributor

This PR is based on #29. For more details about the changes, see the commit messages.

Important: This is still lacking one critical feature. I re-add this feature, when we have decided how to implement it. For discussion see #25. So: Do not merge this PR yet ;)

Many functions were removed by inlining them into their calling
function. Unnecessary function splitting has lead to many small
functions that don't need to be seperated. As a result the code
should be shorter and less spammed with function declarations.

A few different things were done, too:
  - the regexes for ignored filenames are not compiled every
    time a file changes anymore
  - some expressions were simplified to shrink the code
  - some things were renamed to match a more idiomatic
    naming scheme
  - comments were added

Important: Currently the feature of waiting 2 seconds before
starting a new compile is removed! It is planned to be replaced
by a better way of scheduling. However, this was not yet
implemented. This can lead to excessive process spawning when
many files change in a short time. Do not use in production!
@passcod passcod changed the title Big rewrite of the application WIP: Big rewrite of the application Jan 30, 2016

// TODO: check if another command is still running!
let _ = thread::spawn(move || {
debug!("Starting a compile");
Copy link
Member

Choose a reason for hiding this comment

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

This made sense in the original implementation which could only compile, but now should probably be changed to Starting a command run! or something more appropriate.

pub const MAX_ANCESTORS: u32 = 10;

/// Which subdirectories are being watched for changes
pub const WATCH_DIRS: [&'static str; 3] = ["src", "tests", "benches"];
Copy link
Member

Choose a reason for hiding this comment

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

#33 could just be fixed right there, while we're at it.

@passcod
Copy link
Member

passcod commented Jan 30, 2016

LGTM so far.

@ghost
Copy link

ghost commented Jan 30, 2016

LGTM

@LukasKalbertodt
Copy link
Contributor Author

Ok, that should work. Normal running cargo commands, like test and build will prevent spawning of a new process. But a running cargo run will be killed and the new process is spawned. I tested it a few times already, but can't say for sure that my code doesn't contain mistakes still.

Also: it might be desirable to also kill cargo build and so forth on a file change after some time has elapsed since the last file change. But this should be fairly easy to add on top of this.

Another note: it doesn't immediately help with webservers, since killing a process doesn't immediately free it's used ports. Will try more about that later.

Last note: I haven't done profiling of CPU usage due to "busy" timeout waiting yet, but I expect it to be unnoticeable.

println!("$ cargo {}", command);

// Update global information about the running job
if args.get(0) == Some(&"run") {
Copy link
Member

Choose a reason for hiding this comment

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

Will this catch commands such as cargo run -- --some-option ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's already split on whitespace, right? I figured it should be pretty safe. I don't see when this wouldn't work.

In your example, the original call would be cargo watch "run -- --some-option". This would mean that command would be run -- --some-option. Whitespace-split is ["run", "--", "--some-option"].

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, true.

@passcod passcod changed the title WIP: Big rewrite of the application Big rewrite of the application Jan 31, 2016
passcod added a commit that referenced this pull request Jan 31, 2016
Refactor into more idiomatic and cleaner code

Fix #2
Fix #33
Do most of the work towards #25
@passcod passcod merged commit b71a889 into watchexec:master Jan 31, 2016
@passcod
Copy link
Member

passcod commented Jan 31, 2016

Thanks for everything! All merged and released as v3.1.0!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants