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

Remove unused export to JSON feature #1624

Merged
merged 3 commits into from
Feb 10, 2023
Merged

Conversation

Pezmc
Copy link
Contributor

@Pezmc Pezmc commented Jan 30, 2023

This was commented out as part of #547, it may be restored, but for now, let's remove the dead code, this PR can always be reverted down the line.

@Pezmc Pezmc requested a review from hardillb January 30, 2023 09:31
@hardillb
Copy link
Contributor

Following a comment from @knolleary while we were firefighting on Friday night I'm going to suggest we hold off for a moment on this.

@knolleary
Copy link
Member

For context - we found we had no way to take a complete backup of a project. This functionality would have been helpful at the time if it had existed.

As part of the lessons learnt from Friday, we may want to prioritise restoring this functionality.

An alternative is to add a Download option to snapshots - which would have its own set of benefits such as enabling 'offline' device agent. If we go that route, then we can and should strip out the unused code as per this PR.

The point being, let's not remove a bunch of code today that we may, within days, decide we need to restore.

@Pezmc
Copy link
Contributor Author

Pezmc commented Jan 30, 2023

Is there an issue to follow for the possible restoration?

@hardillb
Copy link
Contributor

We are discussing Friday's incident here https://github.com/flowforge/CloudProject/issues/131

Actions yet to be determined.

@Pezmc
Copy link
Contributor Author

Pezmc commented Jan 30, 2023

Thanks for the issue link. In general I'd strongly lean towards removing early, given restoring the feature is only a revert away. Unused code bloats the codebase, goes out of date introducing bugs, and slows down work, however if it's literally likely to be restored this week, let's leave it for now, and re-visit this PR next week.

@hardillb
Copy link
Contributor

hardillb commented Feb 7, 2023

I'm going to say approve, we can pull it back from the history if we change our minds

@Pezmc Pezmc merged commit 8d45dfd into main Feb 10, 2023
@Pezmc Pezmc deleted the chore-remove-export-project branch February 10, 2023 13:51
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