Skip to content

Refuse to create logfile when the name doesn't end in .spl#16

Open
nresare wants to merge 1 commit into
spotify:masterfrom
nresare:refuse_non_spl_log
Open

Refuse to create logfile when the name doesn't end in .spl#16
nresare wants to merge 1 commit into
spotify:masterfrom
nresare:refuse_non_spl_log

Conversation

@nresare

@nresare nresare commented Jan 15, 2014

Copy link
Copy Markdown
Contributor

Instead of just complaining :)

@nailor

nailor commented Jan 15, 2014

Copy link
Copy Markdown

Just out of curiosity: does the implementation really rely somewhere, in some client lib, that the file ends up in the .spl?

@nresare

nresare commented Jan 15, 2014

Copy link
Copy Markdown
Contributor Author

I don't think so. It's more a convenience for new users that the writehash CLI tool expects an .spl. API users are on their own and can call the files whatever they like.

@nailor

nailor commented Jan 15, 2014

Copy link
Copy Markdown

I still kind of liked the warning mode, instead of completely blocking you to write a file if the extension is wrong

@spkrka

spkrka commented Jan 15, 2014

Copy link
Copy Markdown
Member

It's somewhat mixed:

  • The C and Python API:s don't assume any specific file endings. You can use what you want.
  • The get operation on commandline assumes .spi and .spl file endings, mostly so you don't have to input both files.
  • The writehash operation on commandline assumes .spi and .spl in order to be able to know what the call the newly created index file.
  • The java API mostly enforces the .spi and .spl file endings.

I am not sure what approach is best and if we should enforce it everywhere.

@nailor

nailor commented Jan 15, 2014

Copy link
Copy Markdown

But if this is already the behavior in command line, then forcing this makes sense IMO.

What to do with the APIs, I don't know

@rohansingh

Copy link
Copy Markdown
Collaborator

I'm a big fan of being able to name files whatever you want.

@spkrka

spkrka commented Jan 15, 2014

Copy link
Copy Markdown
Member

I think I may agree. I think I'll leave the commandline tool to be as permissive as already is, and possibly also extend the Java API to be able to specify explicit filenames instead of following the convention (as soon as someone actually decides they have that need).

The commands that already assume the convention only do so to simplify the usage, if someone strictly wants some other filenames they can write a minimal commandline tool using the python (or c) api.

@nresare

nresare commented Jan 16, 2014

Copy link
Copy Markdown
Contributor Author

I'm fine with a warning as well (or the current behavior)

@nresare

nresare commented Jan 16, 2014

Copy link
Copy Markdown
Contributor Author

If we decide to go with freedom to the user (sounds nice, doesn't it?) I would propose that we changed writehash to not require .spl.

@spkrka

spkrka commented Jan 16, 2014

Copy link
Copy Markdown
Member

Yes, but to do that, the commandline tool needs to take one additional argument to know both the log file and the index file.

Maybe we could add an optional parameter to specify index file, and if that's specified we drop the requirement that the log file ends with .spl

@Qix-

Qix- commented Aug 13, 2016

Copy link
Copy Markdown

I'm also in the camp of naming the files whatever you want. It's either enforce it library-wide, though, or don't enforce it at all. As a new Sparkey user, I ran into this as well - I could create a new logfile with whatever name I wanted, but then when I went to write to it it errored out. Was confused for a moment and it seemed less elegant than it probably should.

@spkrka

spkrka commented Aug 14, 2016

Copy link
Copy Markdown
Member

This PR is getting a bit stale so we can't merge it regardless, until it's been updated to latest master.

I am not opposed to letting you choose filenames, but we'll need to fix that in all places so it becomes consistent. For backwards compatibility this means we should add more options to the command line for the affected

For sparkey get we need to add sparkey get [-l logfile] indexfile <key> instead of assuming that the log file ends with .spl.

For sparkey writehash we need to add sparkey writehash [-i indexfile] logfile instead of assuming that the index file ends with .spi.

For sparkey rewrite we need to add support for sparkey rewrite --input-index index-file --input-log log-file --output-index index-file --output-log log-file in addition to the existing syntax.

I think this problem only affects the commandline utility, the internals should already be explicit about these things.

I don't have much time at the moment to do this, but if someone else does I am happy to review it.

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.

5 participants