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

make pipeline cleaner #33

Merged
merged 1 commit into from
Oct 5, 2019
Merged

Conversation

johnnychen94
Copy link
Member

@johnnychen94 johnnychen94 commented Oct 1, 2019

The first commit flattens the nested code structure by removing the try-catch block. There're no function changes except using isfile to check if reference file exists. (Is there any special reason using try-catch here?)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 84.605% when pulling 1764e98 on johnnychen94:pipeline into 31901ca on Evizero:master.


return nothing # skip current test case
end

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# File exists:

@oxinabox
Copy link
Member

oxinabox commented Oct 1, 2019

Nice, good improvement.
Can also split the content of the two branchs of the if into their own functions.
One for handing non-existant file (handle_nonextistant ?), and one for handling disagreeing contents (handle_disagreement ?).

@johnnychen94 johnnychen94 changed the title [WIP] make pipeline cleaner make pipeline cleaner Oct 5, 2019
@johnnychen94 johnnychen94 changed the title make pipeline cleaner [WIP] make pipeline cleaner Oct 5, 2019
@johnnychen94 johnnychen94 changed the title [WIP] make pipeline cleaner make pipeline cleaner Oct 5, 2019
@johnnychen94
Copy link
Member Author

johnnychen94 commented Oct 5, 2019

Mind to merge this? I'd like to open a new PR based on this so that it's easier to be reviewed.

@oxinabox
Copy link
Member

oxinabox commented Oct 5, 2019

I stand by my above comments re breaking it into more functions.

@johnnychen94
Copy link
Member Author

Ahh, yes, but I have"better" ideas than simply breaking it into parts, and that would change the code structure.

@johnnychen94
Copy link
Member Author

Or I could push commits into this branch if you like.

@oxinabox
Copy link
Member

oxinabox commented Oct 5, 2019

Ok lets merge this first.

@oxinabox oxinabox merged commit c149378 into JuliaTesting:master Oct 5, 2019
@johnnychen94 johnnychen94 deleted the pipeline branch October 5, 2019 09:19
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