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

Assignment Definition Changes #47

Merged
merged 18 commits into from
Oct 16, 2024
Merged

Assignment Definition Changes #47

merged 18 commits into from
Oct 16, 2024

Conversation

01Parzival10
Copy link
Contributor

Instead of using the WebEditor specific behavior definition, assignments are now defined like they are in the DFD-MetaModel.

Former set-Actions now work like this:
Assignment({input_Pins};Term;{out_Label}) e.g. Assignment({data, request};Sensitivity.Personal || TRUE;{Sensitivity.Public})

forwarding now like this:
Forwarding({input_pins})
e.g. Forwading({data|request, encrypt}) [with data|request being the pin where both data and request arrive]

Auto-Completion and validation have been adjusted

@01Parzival10 01Parzival10 added the enhancement New feature or request label Oct 8, 2024
@01Parzival10 01Parzival10 self-assigned this Oct 8, 2024
Copy link
Contributor

@uuqjz uuqjz left a comment

Choose a reason for hiding this comment

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

I have three suggestions for changing the regex, mainly disallowing whitespace for simplicity and some consistency for naming and using delimiters

src/features/dfdElements/outputPortBehaviorValidation.ts Outdated Show resolved Hide resolved
src/features/dfdElements/outputPortBehaviorValidation.ts Outdated Show resolved Hide resolved
src/features/dfdElements/outputPortBehaviorValidation.ts Outdated Show resolved Hide resolved
Copy link
Member

@Nicolas-Boltz Nicolas-Boltz left a comment

Choose a reason for hiding this comment

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

This regex requires a comment that really explains what it does

@uuqjz
Copy link
Contributor

uuqjz commented Oct 9, 2024

Maybe we could also use constants for the delimiters (,|) in case we ever change them

@01Parzival10
Copy link
Contributor Author

01Parzival10 commented Oct 9, 2024

Maybe we could also use constants for the delimiters (,|) in case we ever change them

Then you'll have to build the Regex from that
I think we should do it for the Pipe but I see no reason to change the comma as list separator

EDIT:
Or maybe leave it all together. Since pipes need to be escaped one would need to touch the Regex either way when they replace the delimiter

@01Parzival10
Copy link
Contributor Author

01Parzival10 commented Oct 9, 2024

This regex requires a comment that really explains what it does

Agreed. I added documentation

@uuqjz
Copy link
Contributor

uuqjz commented Oct 9, 2024

You still allow | pipes in labels...

@01Parzival10
Copy link
Contributor Author

You still allow | pipes in labels...

Fixed that

Copy link
Contributor

@uuqjz uuqjz left a comment

Choose a reason for hiding this comment

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

LGTM

@uuqjz
Copy link
Contributor

uuqjz commented Oct 11, 2024

There are issues with your term validation
!Sensitivity.Pe..rs_ is marked as valid
! is marked as valid
These two i found by trying there might be more

@01Parzival10
Copy link
Contributor Author

There are issues with your term validation
!Sensitivity.Pe..rs_ is marked as valid
! is marked as valid

The same issue existed beforehand. I just adjusted the Editor to the new logic there

@01Parzival10
Copy link
Contributor Author

There are issues with your term validation
!Sensitivity.Pe..rs_ is marked as valid
! is marked as valid

The same issue existed beforehand. I just adjusted the Editor to the new logic there

At least the ! Issue. I'll take care of the label issue

@01Parzival10
Copy link
Contributor Author

There are issues with your term validation
!Sensitivity.Pe..rs_ is marked as valid
! is marked as valid

The same issue existed beforehand. I just adjusted the Editor to the new logic there

At least the ! Issue. I'll take care of the label issue

Done @uuqjz

@uuqjz
Copy link
Contributor

uuqjz commented Oct 11, 2024

Sensitivity.Personal. is marked as valid
Sensitivity.Personal.ABC. is too

@01Parzival10
Copy link
Contributor Author

@uuqjz I fixed that but Validation and Suggestions need to be touched by our new WebDev anyway. This code is hotglued together to work

@uuqjz
Copy link
Contributor

uuqjz commented Oct 14, 2024

@Nicolas-Boltz please rereview. Its fine (for now) from my side.
@Kr0nox will at some point have to refactor this.

Copy link
Member

@Nicolas-Boltz Nicolas-Boltz left a comment

Choose a reason for hiding this comment

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

Looks good to me
@01Parzival10 I leave it up to you to merge this.

@01Parzival10 01Parzival10 merged commit 3ce2f47 into main Oct 16, 2024
1 check passed
@01Parzival10 01Parzival10 deleted the assignment branch October 16, 2024 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants