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

Added same imgur and stuff album downloaders characteristics to ChanRipper #185

Merged

Conversation

EraYaN
Copy link
Contributor

@EraYaN EraYaN commented Mar 9, 2015

So now the chanripper also rips imgur albums.

The building error is in the instagram ripper. "SocketTimedOut" doesn't seem to be a code issue.

I tried to make the UI more flexible (proper resizing) but ugh I hate java UI's, I'm too used to the ease of use of WPF. So it wouldn't work.

@EraYaN
Copy link
Contributor Author

EraYaN commented Dec 4, 2015

This one need some cleaning up before you can merge it, doesn't it?

@4pr0n
Copy link
Owner

4pr0n commented Apr 15, 2016

I like this change; but there's conflicts so I can't auto-merge.

@metaprime
Copy link
Collaborator

@EraYaN could you clean this up and resolve conflicts in this PR so I can merge it?

@EraYaN
Copy link
Contributor Author

EraYaN commented Dec 19, 2016

Mmm, I'll need to review and rebase, I think. I can do it somewhere this week. Otherwise over the Christmas break.

@metaprime
Copy link
Collaborator

@EraYaN Thanks! I'd really appreciate it. I'll be checking in frequently.

Copy link
Collaborator

@metaprime metaprime left a comment

Choose a reason for hiding this comment

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

As commented above.

@EraYaN EraYaN force-pushed the feature-betterchanripper-bestpractices branch from 0b444ee to f846e36 Compare January 9, 2017 15:28
@EraYaN
Copy link
Contributor Author

EraYaN commented Jan 9, 2017

I rebased onto master.

@metaprime
Copy link
Collaborator

@EraYaN Thanks!

@metaprime metaprime modified the milestone: on-deck Jan 10, 2017
Copy link
Collaborator

@metaprime metaprime left a comment

Choose a reason for hiding this comment

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

@EraYaN it looks like you didn't resolve the conflicts. This doesn't compile. Would you mind fixing it up?

@EraYaN
Copy link
Contributor Author

EraYaN commented Apr 25, 2017

It compiled in January. Let me have a look.

import com.rarchives.ripme.utils.Utils;
>>>>>>> Added same imgur and stuff album downloaders characteristics to ChanRipper.
=======
>>>>>>> Added isEmpty to History, fixed some best practices in the code.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unresolved conflict


=======

>>>>>>> Added isEmpty to History, fixed some best practices in the code.
Copy link
Collaborator

Choose a reason for hiding this comment

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

unresolved conflict

public void windowDeiconified(WindowEvent e) { trayMenuMain.setLabel("Hide"); }
@Override
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe put these on the same line as the definition to keep the one-line method consistent?

Copy link
Contributor Author

@EraYaN EraYaN Apr 25, 2017

Choose a reason for hiding this comment

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

This is not my code. But in the same style as C style pragma this should probably be above the method.

EDIT: In other spots in the code it is also done as a method decorator (above the method declaration, with the same indent)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have no strong opinion on this. Function over style, IMO. It's fiine as-is.

//TODO implement proper stack trace handling this is really just intented as a placeholder until you implement proper error handling
e.printStackTrace();
} catch (AWTException e) {
//TODO implement proper stack trace handling this is really just intented as a placeholder until you implement proper error handling
Copy link
Collaborator

@metaprime metaprime Apr 25, 2017

Choose a reason for hiding this comment

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

Yeah, that's true, but I think this code pattern is fair though. The point is to catch the exception, print it out so that someone can investigate it from the log, and not actually crash the program. (It is possible to continue the program and just not finish the operation that excepted.)

@metaprime
Copy link
Collaborator

I promise I'll actually test and give feedback as soon as you update this time, rather than letting it sit around forever.

@EraYaN
Copy link
Contributor Author

EraYaN commented Apr 25, 2017

Well I think it's ready again. Unless you want me to remove the //TODO comments.

@metaprime
Copy link
Collaborator

(Apparently as soon as I can is two weeks...)
Anyway, looks good, thanks!

@metaprime metaprime merged commit 441790b into 4pr0n:master May 9, 2017
@metaprime metaprime mentioned this pull request May 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants