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/3193/custom config todo due #3196

Merged
merged 8 commits into from Jan 13, 2023
Merged

Conversation

MW-Friedrich
Copy link
Contributor

@MW-Friedrich MW-Friedrich commented Jan 12, 2023

The removal of the camera attribute migration for custom view files is due.

Issue: #3193

Description

In #2880 the way camera position and angle were saved changed in an incompatible way. To give everyone some time to transition, a mapping method was introduced to parse the old camera attributes into the new format.

Now, the time has come for its removal, the camera position from old files will now simply be ignored.
This affects all cc.config files generated with CodeCharta 1.101.1 and earlier.

This PR also updates the babel suite because tests were failing locally on my machine.

update to the babel suite
add proper error handling
add (bad) if condition for tests in customConfigHelper.ts
add proper error handling
add (bad) if condition for tests in customConfigHelper.ts
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.

After some considerations I think that we should just remove the transform and a check if the config is conform with the supported config strucutre is also not needed as long as the system does not crash. If it would crash we should handle the error gracefully.

What is happening when loading/importing a config with old camera properties? I guess, they are just ignored and a default camera position is set instead. in that case, I would tolerate this because such deprecated configs must have been created before releasing the feature as stable (so we do not necessarily have to support the transformation of "unstable" configs).

The reason for rollbacking the before discussed solution is that in the future we should handle backwards incompatible config structure changes by the config structure api version and add a appropriate validation method for applying/importing configs.

It looks like this has been forgotten in #2880, to better signal incompatibility it is now changed
also revert changes to applyCustomConfigButton.component.ts and uploadCustomConfigButton.component.ts
@MW-Friedrich
Copy link
Contributor Author

After some considerations I think that we should just remove the transform and a check if the config is conform with the supported config strucutre is also not needed as long as the system does not crash. If it would crash we should handle the error gracefully.

What is happening when loading/importing a config with old camera properties? I guess, they are just ignored and a default camera position is set instead. in that case, I would tolerate this because such deprecated configs must have been created before releasing the feature as stable (so we do not necessarily have to support the transformation of "unstable" configs).

The reason for rollbacking the before discussed solution is that in the future we should handle backwards incompatible config structure changes by the config structure api version and add a appropriate validation method for applying/importing configs.

The changes have been made, and everything seems to run bug free on my machine. The camera position is now simply ignored.

I have also bumped the api version of the config files to better signalise the change.

@MW-Friedrich MW-Friedrich marked this pull request as ready for review January 13, 2023 08:45
@ce-bo
Copy link
Collaborator

ce-bo commented Jan 13, 2023

Let's not bump the version because the deprecation of theese properties occurred when the Custom Views feature was not released stable. It is not ideal that we are already at version 1.0.0. Before releasing the feature as stable the structure should have been at 0.x.x.

Any change that will break the support of a config from now on is going to bump the mature version, because the feature is stable.

@sonarcloud
Copy link

sonarcloud bot commented Jan 13, 2023

[CodeCharta Analysis] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Jan 13, 2023

[CodeCharta Visualization] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

95.7% 95.7% Coverage
0.0% 0.0% Duplication

@MW-Friedrich MW-Friedrich merged commit a5842c2 into main Jan 13, 2023
@MW-Friedrich MW-Friedrich deleted the fix/3193/customConfig-todo-due branch January 13, 2023 09:48
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.

None yet

3 participants