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

Making all JSON Config properties case insensitive #107

Closed
wants to merge 0 commits into from

Conversation

odeke-em
Copy link

This addresses issue #72

@odeke-em
Copy link
Author

odeke-em commented Oct 1, 2014

@bslatkin any free time for a code review? :)

@@ -36,6 +36,7 @@
from dpxdt.client import process_worker
from dpxdt.client import timer_worker
from dpxdt.client import workers
import preprocess_argv
Copy link
Owner

Choose a reason for hiding this comment

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

Can you make this a global import instead of a relative one?

from dpxdt.tools import preprocess_argv

Copy link
Author

Choose a reason for hiding this comment

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

For sure, that has been changed.

@bslatkin
Copy link
Owner

I'm sorry for the delay in reviewing this!!! Will leave comments this week. Thanks for the pull request!

@odeke-em odeke-em force-pushed the master branch 2 times, most recently from 7b54aef to f0065d3 Compare October 28, 2014 06:06
@odeke-em
Copy link
Author

No worries, I totally understand how busy things can get.

@@ -3,17 +3,17 @@
source common.sh

./dpxdt/runserver.py \
Copy link
Author

Choose a reason for hiding this comment

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

Modified the config to illustrate the changes in effect.

@odeke-em
Copy link
Author

@bslatkin ping!

@bslatkin
Copy link
Owner

Hey Emmanuel!

I'm really sorry for the delay here. I looked through this again. I think there was some misunderstanding about what the intention of #72 was. It wasn't to make the FLAGS values case insensitive. I'd like to keep those as they are, so this patch isn't going to work :(

Instead, what we wanted to do was make the config.json file so you can specify clipRect, cliprect, or CLIPRECT and it all works the same way. This is the format I'm talking about:

https://github.com/bslatkin/dpxdt#format-of-config

Sorry for putting off this review for so long. I felt bad about telling you that this wasn't the intention of the original issue. If you're not interested in making the changes no worries. Thanks for your patience.

-Brett

@odeke-em
Copy link
Author

Hello Brett,
Thank you for getting back to me. No worries, I can only imagine how busy you've been as you worked to finish "Effective Python" the book (congrats btw!), as well as kicking ass with the day job.
Thank you for the recommendations and direction, for sure I'd be happy to spin another PR. Feel free to close this PR and you can assign the issue to me and I'll work on it.

Cheers,
Emmanuel

@bslatkin
Copy link
Owner

Thanks for being so understanding and the kind words. I really appreciate 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.

2 participants