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 graph building form text mode #1693

Merged
merged 1 commit into from
Dec 16, 2022

Conversation

gayathrir11
Copy link
Contributor

Summary

Closes #966

How did you test this change?

  • Test(s) added
  • Manual testing (please provide screenshots/recordings)
  • No testing (please provide an explanation)

@changeset-bot
Copy link

changeset-bot bot commented Nov 16, 2022

🦋 Changeset detected

Latest commit: 7f367da

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 27 packages
Name Type
@finos/legend-application-studio Minor
@finos/legend-graph Minor
@finos/legend-application-studio-bootstrap Patch
@finos/legend-extension-dsl-data-space Patch
@finos/legend-extension-dsl-diagram Patch
@finos/legend-extension-dsl-mastery Patch
@finos/legend-extension-dsl-persistence-cloud Patch
@finos/legend-extension-dsl-persistence Patch
@finos/legend-extension-dsl-service Patch
@finos/legend-extension-dsl-text Patch
@finos/legend-extension-format-morphir Patch
@finos/legend-extension-store-flat-data Patch
@finos/legend-extension-store-relational Patch
@finos/legend-extension-store-service-store Patch
@finos/legend-application-pure-ide Patch
@finos/legend-application-query-bootstrap Patch
@finos/legend-application-query Patch
@finos/legend-application-taxonomy-bootstrap Patch
@finos/legend-application-taxonomy Patch
@finos/legend-application Patch
@finos/legend-extension-format-json-schema Patch
@finos/legend-manual-tests Patch
@finos/legend-query-builder Patch
@finos/legend-application-studio-deployment Patch
@finos/legend-application-pure-ide-deployment Patch
@finos/legend-application-query-deployment Patch
@finos/legend-application-taxonomy-deployment Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@finos-cla-bot finos-cla-bot bot added the cla-present CLA Signed label Nov 16, 2022
@codecov
Copy link

codecov bot commented Nov 16, 2022

Codecov Report

Merging #1693 (7f367da) into master (59290dd) will decrease coverage by 0.13%.
The diff coverage is 12.10%.

@@            Coverage Diff             @@
##           master    #1693      +/-   ##
==========================================
- Coverage   40.52%   40.38%   -0.14%     
==========================================
  Files        1400     1400              
  Lines       62617    62915     +298     
  Branches    14618    14685      +67     
==========================================
+ Hits        25377    25411      +34     
- Misses      37139    37403     +264     
  Partials      101      101              
Impacted Files Coverage Δ
...c/stores/editor-state/FileGenerationViewerState.ts 10.34% <0.00%> (-0.77%) ⬇️
...r-state/element-editor-state/ElementEditorState.ts 37.77% <0.00%> (-1.76%) ⬇️
...graph/src/graphManager/AbstractPureGraphManager.ts 40.00% <ø> (ø)
...aphManager/protocol/pure/v1/V1_PureGraphManager.ts 51.77% <3.33%> (-1.79%) ⬇️
...-application-studio/src/stores/EditorGraphState.ts 33.14% <9.09%> (-5.81%) ⬇️
...udio/src/stores/sidebar-state/LocalChangesState.ts 14.42% <10.89%> (-1.26%) ⬇️
...application-studio/src/stores/ExplorerTreeState.ts 60.81% <13.63%> (-6.97%) ⬇️
...ication-studio/src/stores/EditorTabManagerState.ts 42.55% <15.38%> (-10.23%) ⬇️
...lication-studio/src/stores/ChangeDetectionState.ts 63.70% <21.62%> (-7.39%) ⬇️
...egend-application-studio/src/stores/EditorStore.ts 49.08% <25.00%> (-0.66%) ⬇️
... and 1 more

Copy link
Member

@MauricioUyaguari MauricioUyaguari left a comment

Choose a reason for hiding this comment

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

half way through the changes will continue tomorrow

@gayathrir11 gayathrir11 force-pushed the config branch 5 times, most recently from a0bcb70 to 2807139 Compare December 14, 2022 11:26
@gayathrir11 gayathrir11 deleted the config branch December 14, 2022 11:34
@gayathrir11 gayathrir11 restored the config branch December 14, 2022 11:37
@gayathrir11 gayathrir11 reopened this Dec 14, 2022
@gayathrir11 gayathrir11 force-pushed the config branch 4 times, most recently from 417f334 to 77e19b2 Compare December 15, 2022 07:42
@gayathrir11 gayathrir11 marked this pull request as ready for review December 15, 2022 10:35
Copy link
Member

@MauricioUyaguari MauricioUyaguari left a comment

Choose a reason for hiding this comment

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

still got 2/3 more files to go over but I think im okay to merge and start testing. i will leave more comments tomorrow and you can address them in the other text mode PR you have up. Thanks

@@ -172,8 +173,11 @@ export const LocalChanges = observer(() => {
editorStore.changeDetectionState.workspaceLocalLatestRevisionState.changes;
const openChange =
Copy link
Member

Choose a reason for hiding this comment

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

Here I think this is just because of current behaviour but i think now we can actually open a diff if they click on it and they are in text mode right?
Either ways if openChange only works for form mode, we should think about disabling the button or some kind of UI indiactor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think they can open diff editor when they are in text mode. It just doesn't open anything. We can diable the button. That would be a good option. This reminds me we need to disable globalTestRunner too.
Will discuss more on that offline

@MauricioUyaguari MauricioUyaguari merged commit bf2487f into finos:master Dec 16, 2022
gayathrir11 pushed a commit to gayathrir11/legend-studio that referenced this pull request Dec 16, 2022
gayathrir11 pushed a commit to gayathrir11/legend-studio that referenced this pull request Dec 19, 2022
gayathrir11 pushed a commit to gayathrir11/legend-studio that referenced this pull request Dec 19, 2022
gayathrir11 pushed a commit to gayathrir11/legend-studio that referenced this pull request Dec 19, 2022
gayathrir11 pushed a commit to gayathrir11/legend-studio that referenced this pull request Dec 19, 2022
MauricioUyaguari pushed a commit that referenced this pull request Dec 22, 2022
Copy link
Member

@MauricioUyaguari MauricioUyaguari left a comment

Choose a reason for hiding this comment

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

more comments

gayathrir11 pushed a commit to gayathrir11/legend-studio that referenced this pull request Jan 10, 2023
gayathrir11 pushed a commit to gayathrir11/legend-studio that referenced this pull request Jan 10, 2023
gayathrir11 pushed a commit to gayathrir11/legend-studio that referenced this pull request Jan 11, 2023
MauricioUyaguari pushed a commit that referenced this pull request Jan 11, 2023
@gayathrir11 gayathrir11 deleted the config branch April 21, 2023 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-present CLA Signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement: remove graph building in text mode to improve performance
2 participants