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

Better auto-renaming #709

Closed
wants to merge 2 commits into from
Closed

Better auto-renaming #709

wants to merge 2 commits into from

Conversation

nmaier
Copy link
Collaborator

@nmaier nmaier commented Jul 18, 2016

having things like "image.jpg.1" when autorenaming happens is only marginally helpful... It is better to rename it to "image.1.jpg" instead, which is what this PR does.
If a file has no extension, it will still become just "file.1".

Added tests for this too, which required making tryAutoFileRenaming public (short of nasty "only make this public in tests" hacks)

@tatsuhiro-t
Copy link
Collaborator

Thank you for PR.
I think we should do this from the beginning. The preservation of extension is nice especially, for torrent or metalink files, and server does not send correct content-type.
Since we have had the current renaming scheme for many years, I'm wondering changing renaming method could potentially affect the user application code, say, a script which expects the certain renaming method.
What do you think about this? Should we care about it? Or just add new flag to change behaviour?

@@ -39,6 +39,35 @@ void RequestGroupTest::testGetFirstFilePath()

CPPUNIT_ASSERT_EQUAL(std::string("/tmp/myfile"), group.getFirstFilePath());

// test file renaming
Copy link
Collaborator

Choose a reason for hiding this comment

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

In my opinion, these tests should go their own function, say, testTryAutoFileRenaming().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no problem

@nmaier
Copy link
Collaborator Author

nmaier commented Jul 19, 2016

I considered if it would break some code, but really, I saw the likelihood of that to be so small that I don't think we should address this. Either way, the benefits outweigh the potential drawback by this wide a margin, that this new behavior should be the default.
Also, the work and maintenance costs of us adding a pref to revert to the old behavior isn't worth it even if we broke a tiny fraction of scripts; it would be easier for authors just to update their scripts and reap the benefits of a proper extension, or patch out the change if they really have to, so IMO there shouldn't be a compat switch/config option either

@nmaier
Copy link
Collaborator Author

nmaier commented Jul 19, 2016

This should do the trick then, @tatsuhiro-t

@nmaier nmaier self-assigned this Jul 19, 2016
@@ -39,11 +41,65 @@ void RequestGroupTest::testGetFirstFilePath()

CPPUNIT_ASSERT_EQUAL(std::string("/tmp/myfile"), group.getFirstFilePath());

// test in-memory
ctx->getFirstFileEntry()->setPath("/tmp/myfile");

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps, this is accidental left over?

@tatsuhiro-t
Copy link
Collaborator

Fair enough. Let's do this and let's see the response from users.
Please merge this PR when you are ready.

@tatsuhiro-t tatsuhiro-t added this to the v1.26.0 milestone Jul 19, 2016
@nmaier
Copy link
Collaborator Author

nmaier commented Jul 19, 2016

ok, I'll address the minor thing you mentioned, squash it together with a more descriptive log message (for the changelog) and push it once I get home

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.

None yet

2 participants