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

allow custom delimiter to be set #11

Merged
merged 2 commits into from
May 11, 2014
Merged

allow custom delimiter to be set #11

merged 2 commits into from
May 11, 2014

Conversation

fawaf
Copy link
Contributor

@fawaf fawaf commented May 4, 2014

No description provided.

@@ -12,6 +12,9 @@
#http://www.geonames.org/login
#GEONAMES_USERNAME="yourUserName"

# Delimiter for the SQL output
#DELIMITER = " | "
Copy link
Owner

Choose a reason for hiding this comment

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

uncomment this, and remove the spaces for the default version

@marcua
Copy link
Owner

marcua commented May 4, 2014

Awesome! Two quick comments

@marcua
Copy link
Owner

marcua commented May 6, 2014

My vote would be to make user-readable errors for both of these issues, and then require the variables to be uncommented. Having two ways to set the default (one is the kwarg and one is the setting) seems like it might otherwise be confusing to the user.

@fawaf
Copy link
Contributor Author

fawaf commented May 6, 2014

I agree with you on the first part, but I am not sure what you mean by the second part. The user is not setting it two ways. The kwarg is in the code, meaning the user cannot modify it (as if it is a C/Java program). The setting is the one that is modifiable, but, in my opinion, does not have to be set. Which is why if the delimiter is commented out in the settings file, the code should still work, meaning it would default to what is set in the code.

@marcua
Copy link
Owner

marcua commented May 7, 2014

The type signature of the function has delimiter = u"|", which would make a developer think that if they don't pass the delimiter kwarg in, the value will be u"|". This would be a wrong assumption if the DELIMITER setting is set. If you prefer to go the route of adding a setting for it, then

  1. remove the delimiter kwarg
  2. make a user-readable error if settings.DELIMITER is missing
  3. make the default settings.DELIMITER have no spaces, just a single pipe

Sound good?

@fawaf
Copy link
Contributor Author

fawaf commented May 8, 2014

I like that better, but does that mean there will be no default for the delimiter? i.e. just error if you do not have DELIMITER set in the config file? I usually think that a sane default, when it makes sense (which I think it does), is better than a user-readable error. Though i do agree that the signature should not have delimiter argument.

@marcua
Copy link
Owner

marcua commented May 9, 2014

It's fine to have a default. If you agree, the steps would be:

  • Make settings.DELIMITER = u"|"
  • Delete the delimiter argument.
  • If settings.DELIMITER isn't set, set the delimiter to u"|"

@fawaf
Copy link
Contributor Author

fawaf commented May 10, 2014

updated to reflect this change.
also please close issue #3 since it was already fixed by @Oshini.

@marcua
Copy link
Owner

marcua commented May 11, 2014

This looks great. Thanks @gnowxilef!

marcua added a commit that referenced this pull request May 11, 2014
allow custom delimiter to be set
@marcua marcua merged commit 1d3faea into marcua:master May 11, 2014
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.

None yet

2 participants