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.

Sign up for GitHub

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

Jump to bottom

Adding order_properties_before to schema #553

Merged
merged 2 commits into from
Oct 17, 2017
Merged

Adding order_properties_before to schema #553

merged 2 commits into from
Oct 17, 2017

Conversation

skizhak
Copy link
Contributor

@skizhak skizhak commented Oct 11, 2017

  • This is to support ordering Properties of model which extends other models.
  • Properties of schema listed under Extends will be ordered after current model properties if they're listed under order_properties_before. so the final order of properties is
    properties of extends (not listed in order_properties_before)
    current properties
    properties of rest of extends model

@nati nati requested review from nati and morrisson October 11, 2017 17:13
@morrisson
Copy link
Contributor

At least, you should add brief doc to schema.md about this meta property. This feature is ok for me, but I think it's enough and clear for me to write down every properties in propertyOrder list.

@skizhak
Copy link
Contributor Author

skizhak commented Oct 11, 2017

yes ill update the doc and add some UT.

@przemyslaw-dobrowolski-cl
Copy link
Contributor

@skizhak please do not commit util/bindata.go which is a generated file

@skizhak skizhak force-pushed the fix-properties-order branch 3 times, most recently from 248ee0f to d0fbe29 Compare October 13, 2017 01:10
przemyslaw-dobrowolski-cl
przemyslaw-dobrowolski-cl suggested changes Oct 13, 2017
View reviewed changes
.gitignore Outdated
@@ -45,3 +45,5 @@ node_modules
.idea
etc/gohan.log
etc/gohan.db

util/bindata.go
Copy link
Contributor

Choose a reason for hiding this comment

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

bindata.go cannot be ignored. See the comments of the following rejected PR:
#496
and notes
#498

Basically, unless bindata is intentionally changed, it is always reverted before commit. The newest version of go-bindata should generate the same file as in repository thus it will remain unchanged.

Copy link
Contributor Author

@skizhak skizhak Oct 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

i got it. thanks for the pointers. but i think this particular commit will require the bindata change to use the new schema field. i will remove the .gitignore but i think i should be committing bindata so people doing 'go get ' install can use this. is that correct?

@skizhak skizhak force-pushed the fix-properties-order branch 2 times, most recently from e63834a to a674cba Compare October 14, 2017 01:36
@przemyslaw-dobrowolski-cl
Copy link
Contributor

@skizhak You're right - in this particular case, bindata must be regenerated and it is correctly added to commit. Approved.

przemyslaw-dobrowolski-cl
przemyslaw-dobrowolski-cl approved these changes Oct 16, 2017
View reviewed changes
Copy link
Contributor

@przemyslaw-dobrowolski-cl przemyslaw-dobrowolski-cl left a comment

Choose a reason for hiding this comment

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

ok

@skizhak
Copy link
Contributor Author

skizhak commented Oct 16, 2017

@przemyslaw-dobrowolski-cl thanks. @morrisson @nati gentle reminder on this PR.

morrisson
morrisson reviewed Oct 17, 2017
View reviewed changes
Copy link
Contributor

@morrisson morrisson left a comment

Choose a reason for hiding this comment

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

Ok, but you can squash two commits into one because those commits are created for the same purpose.

@skizhak
Adding order_properties_before to schema
69c1221 
- This is to support ordering Properties along with the extended schema
  properties.
  properties of schema listed under Extends will be displayed before
  current properties if they're not listed under order_properties_before
- adding updated bindata from make gen
@skizhak skizhak force-pushed the fix-properties-order branch from dd3bde8 to 69c1221 Compare October 17, 2017 04:34
@nati nati merged commit 2ecd69b into cloudwan:master Oct 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@morrisson morrisson morrisson left review comments

@nati nati nati approved these changes

@przemyslaw-dobrowolski-cl przemyslaw-dobrowolski-cl przemyslaw-dobrowolski-cl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@skizhak @morrisson @przemyslaw-dobrowolski-cl @nati