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

Preserve the ordering of the wiki entries #596

Merged
merged 1 commit into from Jul 14, 2016

Conversation

josepht
Copy link
Contributor

@josepht josepht commented Jun 22, 2016

LP:#1591208

Signed-off-by: Joe Talbott joe.talbott@ubuntu.com

@snappy-m-o
Copy link

Can one of the admins verify this patch?

1 similar comment
@snappy-m-o
Copy link

Can one of the admins verify this patch?

@@ -56,6 +57,23 @@ class InvalidEntryError(Exception):
PARTS_FILE = "snap-parts.yaml"


# yaml OrderedDict loading and dumping
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am tempted to just have this initialization in snapcraft/__init__.py

@sergiusens
Copy link
Collaborator

ok to test

maintainer: Jim Doe <jim.doe@example.com>
origin: lp:snapcraft-parser-example
description: example main2
project-part: app1
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a question. Without the ordered dict, this would result in a different order sometimes, right? I mean, would it be unpredictable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my experimentation the order was not predictable. It's my understanding that python dicts make no promises with regard to order.

@snappy-m-o
Copy link

Can one of the admins verify this patch?

3 similar comments
@snappy-m-o
Copy link

Can one of the admins verify this patch?

@snappy-m-o
Copy link

Can one of the admins verify this patch?

@snappy-m-o
Copy link

Can one of the admins verify this patch?

@josepht
Copy link
Contributor Author

josepht commented Jun 30, 2016

retest this please

@come-maiz
Copy link
Contributor

I think this just needs an update/rebase with master. 👍 from me. I'm not sure if Sergio's comment was a blocker, or a wishlist.

@josepht
Copy link
Contributor Author

josepht commented Jul 11, 2016

Can this be merged?

@sergiusens
Copy link
Collaborator

It can but needs an update.

LP:#1591208

Signed-off-by: Joe Talbott <joe.talbott@ubuntu.com>
@kyrofa kyrofa merged commit 72d263c into canonical:master Jul 14, 2016
@josepht josepht deleted the bugs/1591208 branch July 26, 2016 16:21
@josepht
Copy link
Contributor Author

josepht commented Jul 26, 2016

I'm not sure why the 'state.properties' expected type was changed to OrderedDict in the PR. It's currently failing for me (at least sometimes, I'm not sure how it passed the unit tests).

@sergiusens
Copy link
Collaborator

El 26/07/16 a las 18:23, Joe Talbott escribió:

I'm not sure why the 'state.properties' expected type was changed to
OrderedDict in the PR. It's currently failing for me (at least
sometimes, I'm not sure how it passed the unit tests).

It is probably related to module loading and we would need to make the code you did available upon importing snapcraft instead of it being parser specific code


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#596 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABF_5brBcelPud5MhHcLMKR5QJpJmhjTks5qZjSDgaJpZM4I8MtA.

@josepht
Copy link
Contributor Author

josepht commented Jul 26, 2016

I think you're right about the module loading thing. If I do 'runtests.sh unit' they are OrderedDicts but if I just run the 'test_pluginhandler.py' tests via 'python -m unittest ....' they are dicts.

kalikiana pushed a commit to kalikiana/snapcraft that referenced this pull request Apr 6, 2017
LP:#1591208

Signed-off-by: Joe Talbott <joe.talbott@ubuntu.com>
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

5 participants