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

chore(docs): sqlite instructions #827

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

npetzall
Copy link
Member

@npetzall npetzall commented Feb 13, 2022

image

@npetzall
Copy link
Member Author

npetzall commented Mar 7, 2023

It's old and moldy, so a quick review so that it's understandable. Should we include a link to an example db?

Comment on lines +13 to +15
Recommended driver is the xerial driver: |xerial| |br|
Driver can be downloaded from releases |br|
To use the driver specify ``-t sqlite-xerial``
Copy link
Member

Choose a reason for hiding this comment

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

This can be made more concise.
And less "choppy".
It doesn't flow well.

Driver can be downloaded from releases |br|
To use the driver specify ``-t sqlite-xerial``

If connecting without user/pass the ``-sso`` argument can be used.
Copy link
Member

Choose a reason for hiding this comment

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

Not SQLite-specific. Remove.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reasoning would be that SQLite is more common without user/pass than others since it's not an exposed service.

Copy link
Member

Choose a reason for hiding this comment

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

Are we gonna make that evaluation for each database that we support? I think it would be better to create a credentials page a refer to that one.

Comment on lines +19 to +22
Arguments: |br|
``-s`` same as database name
``-cat`` same as database name
``-db`` full path to database file
Copy link
Member

Choose a reason for hiding this comment

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

Misformatted: Each should appear on their own line.

``-cat`` same as database name
``-db`` full path to database file

Example layout on windows: |br|
Copy link
Member

Choose a reason for hiding this comment

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

No reason to be Windows-specific

Copy link
Member Author

Choose a reason for hiding this comment

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

Reasoning is to be as clear as possible and I have more faith in Linux user being able to convert Windows information than Windows users converting Linux information.

This is a bias from my side, i sort of assume Linux users are more comfortable in a terminal than Windows users.

Copy link
Member

Choose a reason for hiding this comment

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

There's a point where trying to be clear just causes confusion. I'm confident that our users can understand an abstract file system (you could even draw it like a tree if you want)

Comment on lines +26 to +28
To avoid possible issues with space and strange characters. |br|
Base directory ``C:\Tmp\sqlite`` |br|
place ``schemaspy-[version].jar`` in the root of that folder. |br|
Copy link
Member

Choose a reason for hiding this comment

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

General instructions

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, is it a problem?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's a maintainability problem. We don't want to repeat this for each database

place ``sqlite-jdbc-[version].jar`` in a subdir ``driver`` |br|
place ``[databasefile].db`` in a subdir ``db`` |br|

If we use chinook.db as an example we will have the following layout::
Copy link
Member

Choose a reason for hiding this comment

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

Should we include a link to an example db?

Sure!

Comment on lines +38 to +39
Place the command prompt in ``C:\Tmp\sqlite`` |br|
Now you can run::
Copy link
Member

Choose a reason for hiding this comment

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

Replace with terminal command

Place the command prompt in ``C:\Tmp\sqlite`` |br|
Now you can run::

java -jar schemaspy-6.1.1-SNAPSHOT.jar -t sqlite-xerial -sso -o out -dp driver -db C:/Tmp/sqlite/db/chinook.db -s chinook -cat chinook
Copy link
Member

Choose a reason for hiding this comment

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

Don't use snapshot.


java -jar schemaspy-6.1.1-SNAPSHOT.jar -t sqlite-xerial -sso -o out -dp driver -db C:/Tmp/sqlite/db/chinook.db -s chinook -cat chinook

Output will be written to ``C:\Tmp\sqlite\out`` |br|
Copy link
Member

Choose a reason for hiding this comment

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

This is clear from the command

Copy link
Member Author

Choose a reason for hiding this comment

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

Depending on the level of the reader i would say.

Copy link
Member

Choose a reason for hiding this comment

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

In that case, do like we do in the README at tell them to browse C:\Tmp\sqlite\out\index.html. But, again, that's general instructions

Place the command prompt in ``C:\Tmp\sqlite`` |br|
Now you can run::

java -jar schemaspy-6.1.1-SNAPSHOT.jar -t sqlite-xerial -sso -o out -dp driver -db C:/Tmp/sqlite/db/chinook.db -s chinook -cat chinook
Copy link
Member

Choose a reason for hiding this comment

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

Split up up the command over several lines from readability.

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

2 participants