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

Fix/2866/ccsh modify behavior 2 #3448

Merged
merged 11 commits into from
Dec 19, 2023
Merged

Conversation

fritschldwg
Copy link
Contributor

@fritschldwg fritschldwg commented Dec 14, 2023

Fix/2866/ccsh modify behavior 2

Issue: #2866

Description

Descriptive pull request text, answering:

  • Interactive dialog of modify prompts user to choose a single action to perform
  • Updated documentation to point out that only a single action at a time can be performed

Definition of Done

A PR is only ready for merge once all the following acceptance criteria are fulfilled:

  • Changes have been manually tested
  • All TODOs related to this PR have been closed
  • There are automated tests for newly written code and bug fixes
  • All bugs discovered while working on this PR have been submitted as issues (if not already an open issue)
  • Documentation (GH-pages, analysis/visualization READMEs, parser READMEs, --help, etc.) has been updated (almost always necessary except for bug fixes)
  • CHANGELOG.md has been updated

@fritschldwg fritschldwg force-pushed the fix/2866/ccsh-modify-behavior-2 branch 3 times, most recently from 1b6002c to eeed3a4 Compare December 14, 2023 12:12
@fritschldwg
Copy link
Contributor Author

Open for discussion:

  • If the user specifies more than one action - should the command be canceled?

Copy link
Contributor

@Nereboss Nereboss left a comment

Choose a reason for hiding this comment

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

should we also change the move-from and move-to flags into one flag? from my understanding they are always used together

analysis/filter/StructureModifier/README.md Outdated Show resolved Hide resolved

The edges and blacklist entries associated with moved/removed nodes will be altered as well, while all attribute types will be copied.
Specifying multiple actions in a single command results in only one being performed.\
Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion would be to adjust this to "result in only the first action being performed", that way its more clear what the command will do (given we change the implementation to work that way).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's remove this line. I see no argument to keep it. It should be clear from the documentation above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The command should fail immediately if there is more than one action specified.
Otherwise it will not be obvious that the command is failing.
It will silently continueing with an unexpected behavior.


The edges and blacklist entries associated with moved/removed nodes will be altered as well, while all attribute types will be copied.
Specifying multiple actions in a single command results in only one being performed.\
Edges and blacklist entries associated with moved or removed nodes will be adjusted, and all attribute types will be copied.
Copy link
Collaborator

@ce-bo ce-bo Dec 18, 2023

Choose a reason for hiding this comment

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

Just as an information: Blacklist entries are still part of our latests cc.json structure.
Though, I think we can remove this one as it is not needed anymore. Blacklisted items are stored within a custom view and a user is not able to donwload a modified map with blacklisted pathes anymore since a while.

Copy link
Collaborator

@ce-bo ce-bo left a comment

Choose a reason for hiding this comment

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

Thank you for adressing and fixing this :-)

@fritschldwg fritschldwg force-pushed the fix/2866/ccsh-modify-behavior-2 branch from 152578e to 6f413e9 Compare December 18, 2023 13:14
Copy link

sonarcloud bot commented Dec 19, 2023

Quality Gate Passed Quality Gate passed for 'CodeCharta Visualization'

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link

sonarcloud bot commented Dec 19, 2023

Quality Gate Passed Quality Gate passed for 'CodeCharta Analysis'

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
91.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Contributor

@Nereboss Nereboss left a comment

Choose a reason for hiding this comment

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

LGTM!

@Nereboss Nereboss merged commit c0f6e6b into main Dec 19, 2023
7 checks passed
@Nereboss Nereboss deleted the fix/2866/ccsh-modify-behavior-2 branch December 19, 2023 08:32
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.

3 participants