uuosio / ascdk

MIT License
12 stars 9 forks source link

Potential new way to name tables #65

Closed jafri closed 2 years ago

jafri commented 2 years ago

In the following code, you can see how the developer has to match "results" in the decorator and call to TableStore.

@table("results")
export class Results extends Table {
    constructor (
        public customerId: u64 = 0,
    ) {
        super();
    }

    @primary
    get primary(): u64 {
        return this.customerId;
    }

    static getTable(code: Name): MultiIndex<Results> {
        return new MultiIndex<Results>(code, code, Name.fromString("results"));
    }
}

It would be really nice if you could do this instead to only specify "results" once. We can fix tableName as a required field for a table:

@table
export class Results extends Table {
    static tableName: Name = Name.fromString("results")

    constructor (
        public customerId: u64 = 0,
    ) {
        super();
    }

    @primary
    get primary(): u64 {
        return this.customerId;
    }

    static getTable(code: Name): MultiIndex<Results> {
        return new MultiIndex<Results>(code, code, Results.tableName);
    }
}
learnforpractice commented 2 years ago

That's just a different approach and harder to inspect table names. What we lack now is enough documentation I think, that's what I'm working on.

jafri commented 2 years ago

It just seems like a waste to have to declare it twice and keep it in sync.

In my alternate form, you only have to declare "results" once

learnforpractice commented 2 years ago

You are right. So transform should generate getTable method for developers to avoid mistakenly specifying a wrong table name.

jafri commented 2 years ago

Don't we already do that "new" instead of getTable? The problem with that is that the developer sees an error of "new" not existing and loses typechecking


From: learnforpractice @.> Sent: Sunday, June 5, 2022 11:47:16 PM To: uuosio/ascdk @.> Cc: Syed Jafri @.>; Author @.> Subject: Re: [uuosio/ascdk] Potential new way to name tables (Issue #65)

[△EXTERNAL]

You are right. So transform should generate getTable method for developers to avoid mistakenly specifying a wrong table name.

— Reply to this email directly, view it on GitHubhttps://github.com/uuosio/ascdk/issues/65#issuecomment-1147106750, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADILGLJPQ4C5RJI7DMBPFJTVNWNHJANCNFSM5X6L5WFQ. You are receiving this because you authored the thread.Message ID: @.***>

learnforpractice commented 2 years ago

Yes, so getTable method is some kind of a duplicated method that is used when there are no secondary indexes.

The real problem we are facing is the lack of correctly type checking for generated methods.

learnforpractice commented 2 years ago

To solve this problem completely, we need to develop a new plugin for Visual Studio Code I think.

learnforpractice commented 2 years ago

A not-perfect solution for correctly pass type checking is to generate code with eosio-asc gencode command first and then import the generated code which is shown in mixinproxy example.

https://github.com/uuosio/ascdk/blob/dca2f20aa40cdb3e451bd9a8b41ef417683ea2ce/examples/mixinproxy/package.json#L23

jafri commented 2 years ago

For this specific issue, do you think we can reach a compromise for the transform to auto add a static property to every @table class called "tableName"


From: learnforpractice @.> Sent: Monday, June 6, 2022 12:21:37 AM To: uuosio/ascdk @.> Cc: Syed Jafri @.>; Author @.> Subject: Re: [uuosio/ascdk] Potential new way to name tables (Issue #65)

[△EXTERNAL]

A not-perfect solution for correctly pass type checking is to generate code with eosio-asc gencode command first and then import the generated code which is shown in mixinproxy example.

https://github.com/uuosio/ascdk/blob/dca2f20aa40cdb3e451bd9a8b41ef417683ea2ce/examples/mixinproxy/package.json#L23

— Reply to this email directly, view it on GitHubhttps://github.com/uuosio/ascdk/issues/65#issuecomment-1147132065, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADILGLPUA3WIK5LZHTBWLB3VNWRIDANCNFSM5X6L5WFQ. You are receiving this because you authored the thread.Message ID: @.***>

jafri commented 2 years ago

With this, the mock Table class could have a dummy tableName for type checking, and the transform could add the real tableName which would override the dummy


From: Syed Jafri @.> Sent: Monday, June 6, 2022 12:30:09 AM To: uuosio/ascdk @.>; uuosio/ascdk @.> Cc: Author @.> Subject: Re: [uuosio/ascdk] Potential new way to name tables (Issue #65)

For this specific issue, do you think we can reach a compromise for the transform to auto add a static property to every @table class called "tableName"


From: learnforpractice @.> Sent: Monday, June 6, 2022 12:21:37 AM To: uuosio/ascdk @.> Cc: Syed Jafri @.>; Author @.> Subject: Re: [uuosio/ascdk] Potential new way to name tables (Issue #65)

[△EXTERNAL]

A not-perfect solution for correctly pass type checking is to generate code with eosio-asc gencode command first and then import the generated code which is shown in mixinproxy example.

https://github.com/uuosio/ascdk/blob/dca2f20aa40cdb3e451bd9a8b41ef417683ea2ce/examples/mixinproxy/package.json#L23

— Reply to this email directly, view it on GitHubhttps://github.com/uuosio/ascdk/issues/65#issuecomment-1147132065, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADILGLPUA3WIK5LZHTBWLB3VNWRIDANCNFSM5X6L5WFQ. You are receiving this because you authored the thread.Message ID: @.***>

learnforpractice commented 2 years ago

It can be done I think, but just like other generated methods, developers may confuse about where the real method coming from. Honesty, I don't think extending from a mock Table class is a good idea which is only used to passing type checking but may causing confusion for developers.

jafri commented 2 years ago

I agree there’s already a lot of magic functions, like pack/unpack. Maybe similarly we add getTableName auto generated function

I can add comments to the mock class explaining that the mock code is replaced during build process.


From: learnforpractice @.> Sent: Monday, June 6, 2022 12:39:53 AM To: uuosio/ascdk @.> Cc: Syed Jafri @.>; Author @.> Subject: Re: [uuosio/ascdk] Potential new way to name tables (Issue #65)

[△EXTERNAL]

It can be done I think, but just like other generated methods, developers may confuse about where the real method coming from. Honesty, I don't think extending from a mock Table class is a good idea which is only used to passing type checking but may causing confusion for developers.

— Reply to this email directly, view it on GitHubhttps://github.com/uuosio/ascdk/issues/65#issuecomment-1147146329, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADILGLJYXFJKJKE7AKPFMPTVNWTMTANCNFSM5X6L5WFQ. You are receiving this because you authored the thread.Message ID: @.***>

learnforpractice commented 2 years ago

Ok, I will make transform generating a tableName property for table class. getTableName is duplicated to tableName I think. Besides, I recommend calling new method in getTable method if you use getTable instead of new.

jafri commented 2 years ago

Calling new inside getTable would still run into issue of new not being defined?


From: learnforpractice @.> Sent: Monday, June 6, 2022 1:02:37 AM To: uuosio/ascdk @.> Cc: Syed Jafri @.>; Author @.> Subject: Re: [uuosio/ascdk] Potential new way to name tables (Issue #65)

[△EXTERNAL]

Ok, I will make transform generating a tableName property for table class. getTableName is duplicated to tableName I think. Besides, I recommend calling new method in getTable method if you use getTable instead of new.

— Reply to this email directly, view it on GitHubhttps://github.com/uuosio/ascdk/issues/65#issuecomment-1147163808, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADILGLP72Q42RZSJ5OREVV3VNWWB3ANCNFSM5X6L5WFQ. You are receiving this because you authored the thread.Message ID: @.***>

learnforpractice commented 2 years ago

Calling new inside getTable would still run into issue of new not being defined? ____ From: learnforpractice @.> Sent: Monday, June 6, 2022 1:02:37 AM To: uuosio/ascdk @.> Cc: Syed Jafri @.>; Author @.> Subject: Re: [uuosio/ascdk] Potential new way to name tables (Issue #65) [△EXTERNAL] Ok, I will make transform generating a tableName property for table class. getTableName is duplicated to tableName I think. Besides, I recommend calling new method in getTable method if you use getTable instead of new. — Reply to this email directly, view it on GitHub<#65 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADILGLP72Q42RZSJ5OREVV3VNWWB3ANCNFSM5X6L5WFQ. You are receiving this because you authored the thread.Message ID: @.***>

Yeah, we need a solution to solve these kinds of problems completely.

learnforpractice commented 2 years ago

generate tableName property for table class 7675ad95f7e42bc073df5603048b686d1498509c