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

More tests, add filename to Torrent, fast Database#include? method #14

Merged
merged 8 commits into from Oct 19, 2015

Conversation

@winterthediplomat
Copy link
Contributor

@winterthediplomat winterthediplomat commented Oct 17, 2015

I'm sorry I did not make separated Pull Requests, but I got carried out and pushed too much.

  • more tests
    This PR adds some tests (and travis-ci support!) to yamazaki libraries.
  • add filename to Torrent class
    This modification is needed for some modifications in Miyuki, but it may be useful for several clients who just want to know where are the downloaded torrents.
  • fast Database#include? method
    Track databases will only grow larger and larger, even though the rate is low. It should be noted that clients will perform more searches than insertions, so a faster #include is key to a faster Yamazaki. Yes, 56x faster.
    Unfortunately, the modification requires Array#bsearch, which is available only on ruby 2.x. Let me know if you think it has to be compatible with 1.9.x rubies (still shipped by LTS linux distros), we can get around it.
@RoxasShadow
Copy link
Contributor

@RoxasShadow RoxasShadow commented Oct 17, 2015

I suggest so squash something if you can and eventually fix ruby1.9.3 does not have Array#bsearch that's not actually written in the git-way.

@@ -0,0 +1,5 @@
nguage: ruby

This comment has been minimized.

@RoxasShadow

RoxasShadow Oct 17, 2015
Contributor

is that a typo?

This comment has been minimized.

@winterthediplomat

winterthediplomat Oct 17, 2015
Author Contributor

yes.

nguage: ruby
rvm:
- 2.0.0
- 2.1.2

This comment has been minimized.

@RoxasShadow

RoxasShadow Oct 17, 2015
Contributor

Aren't version like 2.1.0 and 2.2.0 available? Just to be more standard, you know.

This comment has been minimized.

@winterthediplomat

winterthediplomat Oct 17, 2015
Author Contributor

they should be available. will check if something like - latest is available.

This comment has been minimized.

@winterthediplomat

winterthediplomat Oct 17, 2015
Author Contributor

Rox, is the next line in this diff ok for you? >2.2.1

This comment has been minimized.

@RoxasShadow

RoxasShadow Oct 17, 2015
Contributor

Sure it is

@db.count { |t| t[:filename] == filename } > 0
begin
@db.bsearch { |t| t[:filename] >= filename }[:filename] == filename
rescue

This comment has been minimized.

@RoxasShadow

RoxasShadow Oct 17, 2015
Contributor

If i recall correctly, Array#bsearch doesn't raise exception. Am I wrong or am I missing something?

This comment has been minimized.

@winterthediplomat

winterthediplomat Oct 17, 2015
Author Contributor

if Array#bsearch can't find an item, nil will be returned. NilClass does not have the [] operator.
a different way to do it may be

res = @db.bsearch { |t| t[:filename] >= filename }
if res
  res[:filename] == filename
else
  false
end

(update: grammar)

This comment has been minimized.

@RoxasShadow

RoxasShadow Oct 17, 2015
Contributor

Oh, I got it. Well, let's keep that as it is.

@title = title
@description = description
@pub_date = pub_date
@link = link
@index = index
@filename = filename

This comment has been minimized.

@RoxasShadow

RoxasShadow Oct 17, 2015
Contributor

Check the alignment here.

@@ -15,13 +15,16 @@
module Yamazaki
class Torrent
attr_reader :title, :description, :pub_date, :link, :index
attr_accessor :filename
def initialize(title, description, pub_date, link, index = 0, filename="")

This comment has been minimized.

@RoxasShadow

RoxasShadow Oct 17, 2015
Contributor

Please leave a blank line between 18 and 19.

describe '#new' do
context 'when track file does not exist' do
it 'is created correctly' do
File.rm "/tmp/yam.db" if File.exists? "/tmp/yam.db"

This comment has been minimized.

@RoxasShadow

RoxasShadow Oct 17, 2015
Contributor

I suggest to use the current path to avoid platform compatibility issues.

This comment has been minimized.

@winterthediplomat

winterthediplomat Oct 17, 2015
Author Contributor

will do. are you really sure someone will use yamazaki on windows?

This comment has been minimized.

@RoxasShadow

RoxasShadow Oct 17, 2015
Contributor

Absolutely nope, but why losing the support for a platform just for a path? :D

it 'is loaded correctly if `track_file` is empty' do
track_file = prepare_db([])
db = Yamazaki::Database.new(track_file)
expect(db.size).to eq(0)

This comment has been minimized.

@RoxasShadow

RoxasShadow Oct 17, 2015
Contributor

expect(db).to be_empty should work too.

@@ -1 +1,28 @@
require 'yamazaki'
require 'date'
require 'base64'
#require 'random'

This comment has been minimized.

@RoxasShadow

RoxasShadow Oct 17, 2015
Contributor

We can remove this require if we don't need it.


def prepare_db(torrents)
db_file_path = new_dbfile_path()
if torrents.size > 0

This comment has been minimized.

@RoxasShadow

RoxasShadow Oct 17, 2015
Contributor

any?



it 'does not insert the same filename if already present' do
skip 'skip: if needed, a file can be re-downloaded, and we want it to be possible'

This comment has been minimized.

@winterthediplomat

winterthediplomat Oct 17, 2015
Author Contributor

I'd like to discuss this, though. Should we remove the test about duplicates filenames? If needed, we may transform it to check the opposite (filenames not considered unique)

This comment has been minimized.

@RoxasShadow

RoxasShadow Oct 17, 2015
Contributor

I think filenames should be unique. Why we would suppose the contrary?

@RoxasShadow
Copy link
Contributor

@RoxasShadow RoxasShadow commented Oct 17, 2015

For me now we're ok 👍 @hydride0?

@winterthediplomat
Copy link
Contributor Author

@winterthediplomat winterthediplomat commented Oct 17, 2015

Please wait until a patch for this issue is added (test has to be un-skipped and code modified to guarantee uniqueness)

@winterthediplomat
Copy link
Contributor Author

@winterthediplomat winterthediplomat commented Oct 17, 2015

Ok, fixed the filename non-uniqueness. Ok for me too.

@RoxasShadow
Copy link
Contributor

@RoxasShadow RoxasShadow commented Oct 17, 2015

Good, can you also squash something too? There a lots of commits there. Then let's wait for @hydride0.

Torrent.filename: it'll contain the path a torrent was downloaded to.

Add tests: Yamazaki now has more tests, covering the usual behaviour
of the library. A travis.yml has been added too.

`Database#size` returns the number of saved items inside the database.
Switching to Array#bsearch (with database sorting)
makes yamazaki a lot faster (25-56x)
ruby1.9.3 is removed because `Array#bsearch`
is not supported from <2.x ruby releases
Roxas noticed a lot of imperfections and little problems,
they're fixed now.
Filenames should be considered unique in the database,
so duplicates are not allowed. If duplicates are present in
the source file, they won't be deleted after the JSON load
(and it would be very, very time consuming to do this, unless
we switch to hashtables).
Fake database files were created under `/tmp`, and
it may break compatibility with non-posix platform.
Moreover they were not deleted, leaving a lot of random
files around: now they are.
@winterthediplomat winterthediplomat force-pushed the winterthediplomat:master branch from 669ef4f to 686ec74 Oct 17, 2015
@winterthediplomat
Copy link
Contributor Author

@winterthediplomat winterthediplomat commented Oct 17, 2015

squashed and forced things, is it ok for you @RoxasShadow ?

@RoxasShadow
Copy link
Contributor

@RoxasShadow RoxasShadow commented Oct 17, 2015

👍

hydride0 added a commit that referenced this pull request Oct 19, 2015
More tests, add `filename` to Torrent, fast Database#include? method
@hydride0 hydride0 merged commit 3b84f9f into hydride0:master Oct 19, 2015
@winterthediplomat
Copy link
Contributor Author

@winterthediplomat winterthediplomat commented Oct 19, 2015

\o/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants