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

Refactor/remove duplicates #349

Merged
merged 5 commits into from
May 20, 2021
Merged

Refactor/remove duplicates #349

merged 5 commits into from
May 20, 2021

Conversation

Ryoken
Copy link
Contributor

@Ryoken Ryoken commented May 20, 2021

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?

#323
#324
#325

Screenshots (if appropriate)

@Ryoken Ryoken requested a review from macintoshpie May 20, 2021 03:09
Copy link
Contributor

@macintoshpie macintoshpie left a comment

Choose a reason for hiding this comment

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

Wow, ~2k lines less! That's awesome.

Because this was so much I wasn't really able to meticulously look at every change.

To convince myself (and others) that this is a non-breaking change, ie that we only refactored the elements into shared elements for reference, I generated multiple v2.3.0 files using oxygenxml. Below is the workflow I used:

  1. Download v2.3.0 xsd from releases page.
  2. Use the Generate Sample XML Files Tool in OxygenXML. See the config below. I've also attached a zip file of these generated buildingsync files. The important configuration settings are "generate optional elements" and "attributes" are both enabled, with a "preferred number of repetitions" of 2, so we are guaranteed all optional elements. In addition I set the "discard optional elements after nested level" to 100, so I don't think we'd be dropping any optional elements that deep.
    bsync_examples.zip

Screen Shot 2021-05-20 at 1 29 50 PM

Screen Shot 2021-05-20 at 1 30 00 PM

Screen Shot 2021-05-20 at 1 30 07 PM

Screen Shot 2021-05-20 at 1 30 15 PM

Screen Shot 2021-05-20 at 1 30 22 PM

3. I validated all of the generated instances against this PR's version of the schema. **None of the instances failed validation.**
from pathlib import Path
import xmlschema

instances = (Path.home() / 'Downloads/bsync_examples').rglob('*.xml')
schema_path = 'BuildingSync.xsd'
schema = xmlschema.XMLSchema(str(schema_path))

for i, instance in enumerate(instances):
    print(f'Validating {instance}')
    try:
        xmlschema.validate(str(instance))
    except Exception as e:
        print(f'FAILED {instance}')
        raise e

I feel 99% confident this is a non-breaking change. In the future it might help to break these changes into separate PRs.

@macintoshpie macintoshpie requested a review from nllong May 20, 2021 19:40
@nllong
Copy link
Member

nllong commented May 20, 2021

Can you resolve the conflict por favor?

Copy link
Member

@nllong nllong left a comment

Choose a reason for hiding this comment

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

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?

@Ryoken
Copy link
Contributor Author

Ryoken commented May 20, 2021

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
Copy link
Contributor

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
Copy link
Contributor Author

Ryoken commented May 20, 2021

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
Copy link
Contributor Author

Ryoken commented May 20, 2021

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!

@Ryoken Ryoken merged commit 24e8744 into develop May 20, 2021
@Ryoken Ryoken deleted the refactor/remove-duplicates branch May 20, 2021 22:16
@JieXiong9119
Copy link
Contributor

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

@macintoshpie
Copy link
Contributor

macintoshpie commented May 25, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants