-
-
Notifications
You must be signed in to change notification settings - Fork 70
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
Recursive up-to-date checks (ref #614). #627
Conversation
0298171
to
c609f3d
Compare
c609f3d
to
2edb357
Compare
Codecov Report
@@ Coverage Diff @@
## develop #627 +/- ##
===========================================
- Coverage 80.20% 80.19% -0.01%
===========================================
Files 69 69
Lines 10326 10332 +6
===========================================
+ Hits 8282 8286 +4
- Misses 2044 2046 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this together!
A couple comments and a request for an extra test, but otherwise this looks great!
os.path.getmtime(included_file) > exe_time | ||
for included_file in included_files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even after using src_info
, this still needs to take include_paths
into account, no?
Oh, actually, I just checked and src_info
returns the actual included file paths, not just the relative paths. That's much better.
Regardless, I think it is still worth adding a test for an include which uses include_paths
set to something non-default.
27a59a2
to
cb356a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me!
Two comments in the test, but otherwise I think this is ready to merge. Thanks again!
As an afterthought, it turns out that
|
That’s no worse that the behavior before this PR, right? |
Remove custom compile checks (ref stan-dev/cmdstanpy#627).
True, maybe something to fix another time. |
Remove custom compile checks (ref stan-dev/cmdstanpy#627).
Submission Checklist
Summary
This PR addresses #614 by running up-to-date checks recursively on included files.
Copyright and Licensing
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): Harvard University
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: