search

BuildingSync / schema

BuildingSync® Schema
https://buildingsync.net
Other
23 stars 22 forks source link

Refactor/remove duplicates #349

Closed Ryoken closed 3 years ago

Ryoken commented 3 years ago

Any background context you want to provide?

There are duplicate elements that need to be removed.

What does this PR do?

Consolidates all duplicate elements.

How should this be manually tested?

bundle exec rake should pass

What are the relevant tickets?

https://github.com/BuildingSync/schema/issues/323 https://github.com/BuildingSync/schema/issues/324 https://github.com/BuildingSync/schema/issues/325

Screenshots (if appropriate)

nllong commented 3 years ago

Can you resolve the conflict por favor?

Ryoken commented 3 years ago

Can you resolve the conflict por favor?

Resolved! The only issue was with the list of duplicates in the validator so I don't believe there needs to be any additional testing.

macintoshpie commented 3 years ago

Resolved! The only issue was with the list of duplicates in the validator so I don't believe there needs to be any additional testing.

@Ryoken Looks like some ones we removed snuck back in there, could you remove those again? (left is diff after original spec update, right is diff after post-merge)

Screen Shot 2021-05-20 at 4 05 42 PM

Ryoken commented 3 years ago

This is great. Thanks Ted for the comprehensive non-breaking change check.

This is going to be a doozy to merge into develop-v3, I expect.

Also, can one of you confirm that Selection Tool is still happy parsing this schema?

I just ran it through the Selection Tool again, added it as a new schema and validated a few sample XMLs with it.

Ryoken commented 3 years ago

Resolved! The only issue was with the list of duplicates in the validator so I don't believe there needs to be any additional testing.

@Ryoken Looks like some ones we removed snuck back in there, could you remove those again? (left is diff after original spec update, right is diff after post-merge)

Screen Shot 2021-05-20 at 4 05 42 PM

You're right, it looks like a recent merge added a couple new duplicates. I pulled those out and updated the PR!

JieXiong9119 commented 3 years ago

Should we bring this to develop-v3 branch as well?

macintoshpie commented 3 years ago

@JieXiong9119 yes, can you open that PR? Not sure how messy that'll be, happy to help if it gets weird