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

Corrected Generation Of Entity Relationship Diagram #153

Merged
merged 10 commits into from
Aug 11, 2024

Conversation

tslever
Copy link
Contributor

@tslever tslever commented Aug 9, 2024

Specifically, I corrected inverted relationships following Christophe's comment on 08/05/2024 11:31:34Z at https://stackoverflow.com/questions/78776709/drawing-relationships-in-entity-relationship-diagram/78831819?noredirect=1#comment138990567_78831819.

Before I made these changes an Entity Relationship Diagram was

Entity_Relationship_Diagram

After I made these changes an Entity Relationship Diagram was

image

@tslever tslever mentioned this pull request Aug 9, 2024
Copy link
Owner

@R0tenur R0tenur left a comment

Choose a reason for hiding this comment

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

First of: big thanks for your contribution!

Project does not build in the CI anymore.

Why was switching from yarn to npm a part of this PR?
Why was debugging docs and frontend docs removed from the docs.md?
Why is docs.md now Windows specific?

docs/docs.md Outdated
- A recent version of node.js
- yarn or npm
## Prerequisites For Generating vsix File
- Windows 11 Home, Version 22H2, OS build 22621.3880, Windows Feature Experience Pack 1000.22700.1020.0
Copy link
Owner

Choose a reason for hiding this comment

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

This is not true. I have never built this on a windows machine and the CI is running Linux.

Copy link
Contributor Author

@tslever tslever Aug 9, 2024

Choose a reason for hiding this comment

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

@R0tenur , I removed this prerequisite.

docs/docs.md Outdated

## Installation
In a terminal:
In Git Bash, ensure you are using Node.js version 20.16.0 by running `node --version`.
Copy link
Owner

Choose a reason for hiding this comment

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

Why Windows specific? Data studio is platform independent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@R0tenur , I generalized docs/docs.md from being Windows specific.


This will generate an vsix that can be installed in azure data studio.

## Debug
Copy link
Owner

Choose a reason for hiding this comment

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

Why removing info about debugging and frontend dev?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@R0tenur , I restored info about debugging and using your front end.

}

if (column.constraints.includes("UNIQUE")) {
Copy link
Owner

Choose a reason for hiding this comment

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

includes has better readability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@R0tenur , I used includes instead of indexOf after getting everything working.

@tslever
Copy link
Contributor Author

tslever commented Aug 9, 2024

@R0tenur , hopefully changes relating to creating our vsix file on Linux will satisfy your Continuous Integration. Hopefully changes to docs/docs.md will address your concerns.

I restored documentation relating to using yarn to create our vsix file.

I restored your sections on debugging and using your front end.

docs/docs.md is no longer Windows specific.

@tslever
Copy link
Contributor Author

tslever commented Aug 9, 2024

@R0tenur , you can see a passing GitHub action at https://github.com/tslever/Generator_Of_ERDs/actions .

Copy link
Owner

@R0tenur R0tenur left a comment

Choose a reason for hiding this comment

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

yarn.lock are lock files for yarn and package-lock.json are lock files for npm.
Since project is using yarn the package-lock files should not be included.

Other than this I think ot looks all fine!

@tslever
Copy link
Contributor Author

tslever commented Aug 10, 2024

@R0tenur , I have deleted files named package-lock.json.

Copy link
Owner

@R0tenur R0tenur left a comment

Choose a reason for hiding this comment

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

Doc stuff that are to specific

docs/docs.md Outdated

Click the ellipsis and Install from VSIX...

Install file `Generator_Of_ERDs/backend/schema-visualization-0.9.3.vsix`.
Copy link
Owner

Choose a reason for hiding this comment

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

One last thing: "Generator_Of_ERDs" seem to be the name of your fork.

Also the 0.9.3 is the version number that will vary each time

Copy link
Contributor Author

@tslever tslever Aug 11, 2024

Choose a reason for hiding this comment

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

@R0tenur , I referenced a generalized path to the vsix file in an earlier part of docs.md.

docs/docs.md Outdated

Run `npm run pack`.

Move `schema-visualization-0.9.3.vsix` to a desired location for installation as an extension in Azure Data Studio.
Copy link
Owner

Choose a reason for hiding this comment

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

0.9.3 is version that will vara each time

Copy link
Contributor Author

@tslever tslever Aug 11, 2024

Choose a reason for hiding this comment

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

@R0tenur , I put a generalized path to the vsix file here.

Copy link

sonarcloud bot commented Aug 11, 2024

@R0tenur R0tenur merged commit db2e990 into R0tenur:master Aug 11, 2024
1 of 2 checks passed
@andreasmarxer
Copy link

andreasmarxer commented Aug 19, 2024

First thanks for this fix and update!

I would be really interested in this correction of the directions, but after updating from V0.9.1 to the latest version V0.9.4 I receive an error and the diagram generation is not anymore working:
Error loading webview: Error: Could not register service worker: InvalidStateError: Failed to register a ServiceWorker: The document is in an invalid state..

@R0tenur @tslever

Anyone also experiencing this with v0.9.4 or any ideas what could be the issue on my side?

@tslever
Copy link
Contributor Author

tslever commented Sep 7, 2024

@andreasmarxer , hi. You're welcome. Thanks to @R0tenur also.
I am able to generate an ERD.
See ./docs/docs.md.
I again cloned this repository, regenerated the extension using Git Bash and npm, and reinstalled the extension.
I recall ensuring Continuous Integration using Windows Subsystem for Linux and the GitHub Action of my fork of this repository.
Before regenerating the extension, to avoid experiencing dependency hell again, I downloaded the package-lock.json files that I deleted toward the end of my work per @R0tenur's request.

@andreasmarxer
Copy link

Hi @tslever
Thanks for the reply and sorry for the misunderstanding.

I did not build the visx package by my own, I did jsut take the latest version v0.9.4 available in the builds, which should include this change as well. But I will in this case give it a try to generate the visx package locally on my machine.

@tslever
Copy link
Contributor Author

tslever commented Sep 16, 2024

@andreasmarxer , I noticed that this pull request is closed. Would you please create a pull request with a new build available in the appropriate location?

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