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

add OrderedDict for preserving key order #2

Closed
wants to merge 3 commits into from
Closed

Conversation

beygi
Copy link

@beygi beygi commented Mar 2, 2013

No description provided.

@uiri
Copy link
Owner

uiri commented Mar 2, 2013

I don't want to break compatibility with python 2.6 (and potentially earlier) for this. Do you have a compelling argument in favour of ordering dictionaries?

@untitaker
Copy link

@uiri You still can use the ordereddict package from PyPI

@beygi
Copy link
Author

beygi commented Mar 2, 2013

preserving orders is an extra feature and it's useful when we want to doing jobs in orders that exist in toml file so its nice to preserving orders by default

@uiri
Copy link
Owner

uiri commented Mar 2, 2013

preserving orders is an extra feature and it's useful when we want to doing jobs in orders that exist in toml file so its nice to preserving orders by default
I'm not sure I understand the use case. Do you mean iterating over the dictionary as if it were an array? Though, If you can make sure that python 2.6 users are taken care of, I see no reason to reject the patch.

I'm not sure how to handle the dependency of the ordereddict package which @untitaker mentioned.

@untitaker
Copy link

You just can check for the version inside the setup.py and supply the setup function with the dependency if neccessary. This is how i have handled it in untitaker/python-webuntis and it works quite well.

@untitaker
Copy link

Unfortunately the official spec from mojombo/toml doesn't indicate whether hashes have order or not.

@beygi
Copy link
Author

beygi commented Mar 2, 2013

@untitaker i'm not familiar with setuptools , can you fix it ?

@beygi
Copy link
Author

beygi commented Mar 2, 2013

@untitaker i inform mojombo about this issue

@untitaker
Copy link

@beygi Is it okay if i make a pull request to your fork, so it shows up in this one?

@beygi
Copy link
Author

beygi commented Mar 2, 2013

@untitaker its okay , when your work is done i send a new pull request . thank you :)

@untitaker
Copy link

If you merge beygi#1, the changes will show up here.

Use ordereddict from PyPI for Python < 2.7
@untitaker
Copy link

I just tested this because i wasn't sure if distutils behaved as documented -- and surprise, it ignores the requires argument completely. If you allow, i will create another pull request on @beygi's fork, which switches from distutils to setuptools. Is this okay for you @uiri?

@untitaker
Copy link

And it seems that this point has been made unneccessary by the response in toml-lang/toml#162 anyway.

@uiri
Copy link
Owner

uiri commented Mar 2, 2013

Given that mojombo has stated not to preserve order (or at least that it is not a requirement), I'm closing this pull request.

@uiri uiri closed this Mar 2, 2013
@beygi
Copy link
Author

beygi commented Mar 2, 2013

@uiri this is an extra feature ! and its good to keep 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.

3 participants