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

206 fix tag related info msg #218

Closed
wants to merge 5 commits into from
Closed

Conversation

ShiriMoran
Copy link
Contributor

No description provided.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Feb 10, 2025
@ShiriMoran ShiriMoran requested a review from adisos February 10, 2025 14:23
@ShiriMoran ShiriMoran linked an issue Feb 10, 2025 that may be closed by this pull request
@adisos
Copy link
Contributor

adisos commented Feb 10, 2025

I still see several issues:

(1) getting warning such as "expr ((( members IDs: )) of group .... is either illegal or not supported by the POC"
this does not indicate what is the issue with the group expr.. and long id better be removed from the message.

(2) "is either illegal or not supported by the POC" should be rephrased.. can specify which one is relevant? illegal / not supported? + remove "POC" , just say not supported. (should also update doc what is supported / not supported) .

(3) getting many warning duplications

(4) change expr ((Name Of VirtualMachine CONTAINS New)) to expr (Name Of VirtualMachine CONTAINS New)

(5) empty expr str should be removed (expr () of group ... -> change to expr of group...)

(6) expr (to implement) of group should be changed to expr of group...

@ShiriMoran
Copy link
Contributor Author

ShiriMoran commented Feb 10, 2025

I still see several issues:

(1) getting warning such as "expr ((( members IDs: )) of group .... is either illegal or not supported by the POC" this does not indicate what is the issue with the group expr.. and long id better be removed from the message.

Where did you get members IDs? it should be part of the message

I thought that for the POC its enough, and we don't need all the details. The README will include what is supported.
If needed I'll revisit it.

(2) "is either illegal or not supported by the POC" should be rephrased.. can specify which one is relevant? illegal / not
supported? + remove "POC" , just say not supported. (should also update doc what is supported / not supported) .

Same comment as above.

(3) getting many warning duplications

This seems to be #213

(4) change expr ((Name Of VirtualMachine CONTAINS New)) to expr (Name Of VirtualMachine CONTAINS New)

This is result of expr's String() and is not related to this PR. If needed, open another issue.

(5) empty expr str should be removed (expr () of group ... -> change to expr of group...)

This seems another instance of #213

(6) expr (to implement) of group should be changed to expr of group...

Perhaps the best way to handle this is extend expr's string(); lets open an issue for this and for 4

@ShiriMoran
Copy link
Contributor Author

will revisit in a fresh branch after #213 is fixed

@ShiriMoran ShiriMoran closed this Feb 11, 2025
@ShiriMoran ShiriMoran deleted the 206_fix_tag_related_info_msg branch February 11, 2025 10:07
@ShiriMoran ShiriMoran removed a link to an issue Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants