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

Add the option to copy SQL name from the name of a persistent class #1209

Open
evshvarov opened this issue Aug 12, 2023 · 20 comments
Open

Add the option to copy SQL name from the name of a persistent class #1209

evshvarov opened this issue Aug 12, 2023 · 20 comments
Labels
enhancement New feature or request priority/community-opportunity Issues identified as good community contribution opportunities

Comments

@evshvarov
Copy link
Member

evshvarov commented Aug 12, 2023

It's not clear what is the SQL name of a persistent ObjectScript class.
It can be obtained but developers spend several minutes every time to figure out the SQL name of the persistent class.

Let's add a helper "Copy SQL table name" above the Classname similar we have Copy Invocation for a method - this will save a lot of useful time for developers.
Screenshot 2023-08-12 at 1 47 29 PM

@isc-bsaviano
Copy link
Contributor

I don't see a need to add this. The SQL table name of the class is very easy to determine visually from the class definition line.

@gjsjohnmurray
Copy link
Contributor

@evshvarov I agree with @isc-bsaviano.

If adding it is going to make things easier for you I encourage you to create your own extension to add it. This could be a useful learning exercise for you.

https://code.visualstudio.com/api/get-started/your-first-extension is a good place to start and https://github.com/microsoft/vscode-extension-samples/tree/main/codelens-sample is a concrete example of an extension that adds a CodeLens, which is the VS Code name for this kind of UI element.

@isc-bsaviano isc-bsaviano closed this as not planned Won't fix, can't repro, duplicate, stale Aug 16, 2023
@evshvarov evshvarov reopened this Aug 16, 2023
@evshvarov
Copy link
Member Author

evshvarov commented Aug 16, 2023

It is not that obvious as you say. Question to you @gjsjohnmurray @isc-bsaviano - what is the SQL name for the following IRIS class?

Class dc.sample.example.PersistentClass Extends %Persistent?

and for the following class:

Class dc.sample.example.PersistentClass Extends %Persistent [ SqlTableName = table ]

Can you provide it from scratch? It should be easy for you as you say.

And a question along: how many manual operations developer should provide to get the SQL Table name for a given class?

As soon as we introduce more and more products to enable SQL to work with IRIS I think ANY enhancement that will ease the barrier to use SQL for developers will make a lot of sense.

@isc-solon
Copy link
Collaborator

I'm still not getting this. Isn't it enough that each time the developer compiles the class, they'll see:
Compiling table dc_sample_example.PersistentClass
or
Compiling table dc_sample_example.table

@isc-bsaviano
Copy link
Contributor

The user doesn't need to do any "manual operations"; the name of the table follows simple rules. The user could also re-compile the class and see the name in the output channel as Joel said. This issue also assumes that a SQL developer would be looking at class definitions in VS Code at all. I think it's more likely they'd be using a SQL focused tool like DBeaver and in that case they'd see the table name.

@evshvarov
Copy link
Member Author

evshvarov commented Aug 16, 2023

"the name of the table follows simple rules", but still needed to be typed manually.

yes, re-compilation and switching to "Output" tab to get the name that @isc-solon Joel provides could be the workaround, didn't know that.
But it is not obvious - developer need to know in advance that compiler provides this info.

On the "VS Code is not for SQL developer" - it's not 100% true, as VSCode can be used to deal with Embedded SQL, with Embedded Python, where SQL can be used to deal with IRIS classes, and still we have %SQL.Statement ObjectScript class with number of methods to deal with SQL representations of InterSystems classes.

I think SQL access is getting more and more important and developers could benefit from a convenient way as "One click" to get the SQL representation of InterSystems class

@evshvarov
Copy link
Member Author

Also, maybe IRIS class could have such a property with the proper sqlname, but I don't know a such.

@isc-bsaviano
Copy link
Contributor

I still don't understand why this necessary. I don't think it's a burden for users to have to think for a few seconds about a table name and spend another few typing it out.

@evshvarov
Copy link
Member Author

evshvarov commented Aug 16, 2023

@isc-bsaviano "I don't think it's a burden for users to have to think for a few seconds about a table name and spend another few typing it out." - I think this what the helpful IDE stands for - to save developer from thinking of "routine" things, save seconds for a developer and to help to avoid syntax errors, as it is easy to make a syntax mistake while typing especially a long sql name.
We have a multi-model data platform, where should be a clear link of a class and the table it is represented by, and currently there is no any (other then compilation output in some Tab. I think this could be a very helpful enhancement that besides saving time and minimizing errors for a developer it could also confirm multi-model status for InterSystems IRIS.

@isc-bsaviano
Copy link
Contributor

I disagree. I think this would just be UI clutter. Unless @gjsjohnmurray or @daimor want to do this I'm going to close it.

@isc-solon
Copy link
Collaborator

isc-solon commented Aug 16, 2023

My 10 cents: For SQL usage in methods of classes, besides the SQL language itself, developers have to type table names and property names, both of which could be long.

  • If they can't remember a property name (or its SqlFieldName), they have to scroll or use Outline or Find in the class definition to find the property, and double-click or drag over the property name to select it, copy it, and then go back to the method and paste it in.
  • If they can't remember or don't know how to figure out a table name from the top line, they have to look in the Output tab for a recent compilation to find the table name, double-click or drag over the table name to select it, copy it, and then go back to the method and paste it in.

I don't see any difference in the amount of work for typing properties vs typing tables.

But maybe the real issue here is code completion? In VS Code - ObjectScript, we don't have SQL code completion inside class queries or for embedded SQL, and we obviously can never have it for dynamic SQL. We also don't have code completion in the Execute Query tab in the Portal, although there is the Query Builder button (I don't know how popular that is). So maybe developers should use tools like DBeaver or something similar (does the SQL Tools extension provide code completion?) to develop their queries, and then copy the finished query into the method.

@evshvarov
Copy link
Member Author

@isc-solon , on typing properties - you can copy them from properties part of the class
declaration. it's not an option for table name.

@evshvarov
Copy link
Member Author

I disagree. I think this would just be UI clutter. Unless @gjsjohnmurray or @daimor want to do this I'm going to close it.

I'm not insisting on the suggested way of UI implementation, it could be any way that is not a UI clutter". But it's an obvious pain for developers that could be converted into IDE's advantage.

@alexatwoodhead
Copy link

alexatwoodhead commented Aug 16, 2023

Could editor have a "Beginner" mode for class definitions?
We all started somewhere before all "obvious" happened.

SqlTableName is a class keyword. Used to override the default.
In a similar way a SqlFieldName is a property keyword, used to override the default.

So could / should the editor display defaults for SqlTableName and SqlFieldName .

Would be intuitive for the beginner user to know they can be overridden and also how.

@alexatwoodhead
Copy link

How intuitive is it that:

  • Creating a persistent class called User.MyTable is accessed by "SQLUser.MyTable"?
  • The dots in package name need to be replaced by underscore? Expect a few people iterate through trying to use quotes
  • Reserved names need to be quoted
  • You don't need the package name if referring to the current table or another table in the same package

@isc-bsaviano isc-bsaviano added enhancement New feature or request priority/community-opportunity Issues identified as good community contribution opportunities labels Oct 24, 2023
@isc-bsaviano
Copy link
Contributor

Adding the new priority/community-opportunity label since I believe this issue is a good candidate for community contribution. I'll leave it open for at least a few more months to see if anyone wants to open a pull request before closing it.

@WolfenGames
Copy link

WolfenGames commented Aug 26, 2024

Hi,

I'm sorry if this is out of place, or the wrong way to ask the question.
I thought maybe considering that is labeled as community-opportunity I would take a stab at it, if anything as a learning experience of this amazing extension.

I have managed to get something similar to what OP is asking for, however, I have reached a roadblock,
This may be a limitation to my understanding of the matter.
I am trying to determine whether the class or superclasses, inherit from a %Persistent

I have tried looking at the %Dictionary.CompiledClass table, which does get me a list of inherited classes, but I am afraid of the impact of doing too many calls just to determine if it does inherit from %Persistent as it may a bit too heavy to execute the calls if there is a large inheritance tree, although technically I would only have to look at the first one and keep looking until we reach an empty field for Super and return. But this feels like the wrong approach.

TL;DR: Is there an easier way to determine if the current class relies on %Persistent?

Thanks in advance

EDIT:
I suppose one option is to continue scanning the file until we hit a Storage Definition symbol? Would this more preferable? are there cases that this would not work?

@isc-bsaviano
Copy link
Contributor

Hi @WolfenGames, welcome to the community! You can use the %Dictionary.ClassDefinition_SubclassOf() stored procedure to find all subclasses of %Library.Persistent. I suggest you cache the results so you don't have to call it every time you want to determine the SQL table name for a class.

@WolfenGames
Copy link

Thank you @isc-bsaviano, happy to be here.
I shall give that a shot, it is definitely a better approach I believe.

Given the comment by alex, is there a way to determine the following: Reserved names need to be quoted

@isc-bsaviano
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority/community-opportunity Issues identified as good community contribution opportunities
Projects
None yet
Development

No branches or pull requests

6 participants