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

Drools 3619 More property headers can have the same alias #1078

Merged
merged 8 commits into from
Mar 1, 2019

Conversation

gitgabrio
Copy link
Contributor

@kkufova
Copy link
Contributor

kkufova commented Feb 21, 2019

@gitgabrio, it does not work.

bug1

@gitgabrio
Copy link
Contributor Author

@kkufova
Fixed.

@kkufova
Copy link
Contributor

kkufova commented Feb 22, 2019

What we decided:

  • it should not be possible to use the same alias for two properties within the same instance;
  • it should be possible to use the same alias for two properties of two different instances;
  • if a property is renamed in GIVEN, the same property must be renamed in EXPECT and vice versa;
  • it should not be possible to use an alias for a property in GIVEN (Book.name) and the same alias for a different property in EXPECT (Book.copies)
    -- this means that aliases are unique within one instance, regardless of position (GIVEN or EXPECT).

Copy link
Member

@Rikkola Rikkola left a comment

Choose a reason for hiding this comment

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

Minor comment are the years 2018 in new files. I think this is because the files were just moved, but if you need to touch the PR for some other reason perhaps fix them.

@kkufova
Copy link
Contributor

kkufova commented Feb 25, 2019

@gitgabrio, the last requirement is not satisfied:

it should not be possible to use an alias for a property in GIVEN (Book.name) and the same alias for a different property in EXPECT (Book.copies)

@gitgabrio
Copy link
Contributor Author

  1. GIVEN - Book - name -> rename to "huhu"
  2. EXPECT - Book - numberOfCopies -> rename to "huhu"

it should not be possible

@gitgabrio
Copy link
Contributor Author

@kkufova
Klara please retest this ( 😄 )

Copy link
Contributor

@kkufova kkufova left a comment

Choose a reason for hiding this comment

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

Everything works well, I didn't discover any bugs. Approving.

👍

@kkufova
Copy link
Contributor

kkufova commented Mar 1, 2019

@manstis, could you please merge this?

@manstis manstis merged commit 65c9c56 into kiegroup:master Mar 1, 2019
@gitgabrio gitgabrio deleted the DROOLS-3619 branch March 1, 2019 11:11
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.

5 participants