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

Default exposure types to be processed, added logs for skipped files. if not done, do logic now applies to outdir reroute. #496

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

michaelJwilson
Copy link

Added argument (with defaults) for exposure types to be processed; added logs for skipped exposures / arcs; further tweaks for clobber: if not done, do now also applies to outdir aswell as simdir, as should be the case.

…ded logs for skipped exposures / arcs; further tweaks for clobber: if not done, do now also applies to outdir as should be the case.
@michaelJwilson michaelJwilson requested a review from sbailey June 12, 2019 22:20
Copy link
Contributor

@sbailey sbailey left a comment

Choose a reason for hiding this comment

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

Looks fine, with a small but important request: please update your indentation to consistently use 4-spaces instead of 2-spaces or tabs. Thanks. Apologies for the very delayed review. I'd do it myself except this PR is coming from your fork so I don't have write access to your copy, so punting it back to you to add a few spaces here and there.

@sbailey
Copy link
Contributor

sbailey commented Aug 2, 2019

It looks like this PR is superseded by PR #501 which has the same changes, right? Same request for 2-space -> 4-space indentation though, for smoother code collaboration with others.

@moustakas
Copy link
Member

Is this PR still active / valid? @michaelJwilson

@michaelJwilson
Copy link
Author

Unless someone knows better, I believe it is. I can fix the indentation by the end of the week. If you rather I just closed it, let me know.

@sbailey
Copy link
Contributor

sbailey commented Apr 6, 2020

I'm a bit confused. When I last looked at this PR it appeared that it was superseded by PR #501, which just got closed because it was going to be superseded by yet another PR. As long as the indentation is made to consistent 4-spaces, we can proceed with whatever is most convenient: either merging this PR and then having a new one with more functionality, or putting it all into the new one. Regardless, thanks @michaelJwilson and @moustakas for getting these rolling again.

@michaelJwilson
Copy link
Author

I believe the logical thing is to fix this wrap-fastframe problem independently. I'll fix the indentation by the end of the week.

It made no sense to retain #501 as I effectively started again a few weeks ago and can speak to it being what we want. Apologies for the confusion.

@michaelJwilson
Copy link
Author

Added an 'Indentation fixes' commit that should hopefully close this out. I don't see any tab characters left. Thanks.

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