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

WINDUP-683 SetInProperties to store sets in vertices efficiently #622

Merged
merged 1 commit into from Jul 13, 2015
Merged

WINDUP-683 SetInProperties to store sets in vertices efficiently #622

merged 1 commit into from Jul 13, 2015

Conversation

OndraZizka
Copy link
Contributor

No description provided.

@windupgithubbot1
Copy link

Refer to this link for build results (access rights to CI server needed):
http://10.3.8.5//job/windup-pr-builder/893/

Failed Tests: 3

org.jboss.windup.graph:windup-graph-tests: 3


Test FAILed.

@windupgithubbot1
Copy link

Refer to this link for build results (access rights to CI server needed):
http://10.3.8.5//job/windup-core-PR-dependents-check/262/
Test PASSed.

mbriskar
mbriskar reviewed Jul 3, 2015
View reviewed changes
graph/tests/src/test/java/org/jboss/windup/graph/typedgraph/setinprops/SetInPropertiesTest.java
System.out.println(" Frame class: " + frame.getClass());
for (Class<?> iface : frame.getClass().getInterfaces())
{
System.out.println(" Implements: " + iface.getName());
Copy link
Contributor

Choose a reason for hiding this comment

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

this will probably need to go away in order to be merged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are more tests that use System.out...

Copy link
Member

Choose a reason for hiding this comment

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

+1 - println in tests seems useful to me

Copy link
Contributor

Choose a reason for hiding this comment

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

well, I thought it's not a good approach to have them. But if you like it there, no problem :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is useful on an individual basis. I personally wouldn't leave them, but it's also a test so it's not a huge deal unless it really starts mucking up build logs or something.

@mbriskar
Copy link
Contributor

mbriskar commented Jul 3, 2015

If I understood the implementation correctly, I am not sure this is a good approach to go through.
I see several issues that must be handled:

  • In case the prefix is not specified, you may remove in setter some other keys that happend to have the same key as one of the strings in the Set. In case of retrieval, you will return all the keys
  • In case multiple sets have the same prefix, they are handled as one set
  • In order to not face problem already listed, as a user I would need to specify prefix even though it doesn't matter for me what prefix it will be.

In order to face the issues, I would suggest generating unique ID for the vertex by default.

@OndraZizka
Copy link
Contributor Author

Matej, to your points:

  1. That is the point of an empty prefix - to see the property names as a set. May come in handy.

  2. It is up to the user to set up his model properly. One could also argument against other Frames annotations - if the user screws up the property names, Frames will overwrite properties which the user didn't want to. I could add checking of other properties in the class, but that would harm performance.

@windupgithubbot1
Copy link

Refer to this link for build results (access rights to CI server needed):
http://10.3.8.5//job/windup-pr-builder/925/
Test PASSed.

@windupgithubbot1
Copy link

Refer to this link for build results (access rights to CI server needed):
http://10.3.8.5//job/windup-core-PR-dependents-check/295/

Failed Tests: 1

org.jboss.windup.rules:windup-rulesets: 1


Test FAILed.

@mbriskar
Copy link
Contributor

mbriskar commented Jul 8, 2015

1.) Isn't there a method like vertex.properties to get the set of properties or something similar?
2.) Maybe the problem is that from the begining I am looking at this as a seperate property that just happens to store in a Set.

lincolnthree added a commit that referenced this pull request Jul 13, 2015
@lincolnthree
Merge pull request #622 from OndraZizka/setInProps-WINDUP-683
e8c5e56 
WINDUP-683  SetInProperties to store sets in vertices efficiently
@lincolnthree lincolnthree merged commit e8c5e56 into windup:master Jul 13, 2015
PhilipCattanach pushed a commit to PhilipCattanach/windup that referenced this pull request Feb 20, 2023
@jmle
Merge pull request windup#622 from PhilipCattanach/WINDUPRULE-793
09da3ef 
WINDUPRULE-793 - OpenJDK 8 to 11 - sun.reflect.Reflection removed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@OndraZizka @windupgithubbot1 @mbriskar @lincolnthree @jsight