Skip to content

Conversation

@mykalmax
Copy link
Member

@mykalmax mykalmax commented Feb 4, 2023

Add Models for File and Directory
Add ability to add empty sql file to test sql queries

@mykalmax mykalmax marked this pull request as ready for review February 6, 2023 17:41
@mykalmax mykalmax force-pushed the max/editor_start_screen branch from 2659ba4 to 45d9e95 Compare February 7, 2023 19:28
@mykalmax mykalmax requested a review from vchan February 7, 2023 21:03
@tobymao
Copy link
Contributor

tobymao commented Feb 7, 2023

testing sql queries doesn't need to be a file, it should just be in memory

@mykalmax mykalmax changed the title Set starting state for editor as empty sql file Set starting state for editor as empty sql tab Feb 7, 2023
@mykalmax
Copy link
Member Author

mykalmax commented Feb 7, 2023

testing sql queries doesn't need to be a file, it should just be in memory

it just a new tab in editor without saving.
use can add new tab, write query and run/validate and close

@mykalmax mykalmax force-pushed the max/editor_start_screen branch from 80e8116 to 17698c1 Compare February 8, 2023 00:50
@mykalmax mykalmax force-pushed the max/editor_start_screen branch from 15bee6d to 98dc883 Compare February 9, 2023 22:17
@mykalmax mykalmax requested a review from vchan February 9, 2023 22:55
return this._initial;
}

get isModel(): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

What does isModel mean? Are there subclasses that override this function and return false?

Copy link
Member Author

Choose a reason for hiding this comment

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

it just a flag that signaling this object is a model, does not have to be a getter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, but is there a case where isModel is false? How is this flag used?

return this.files.length > 0;
}

get withDirectories(): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

hasDirectories?

Copy link
Member Author

Choose a reason for hiding this comment

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

i used with for similar cases in other places , so juts for consistency . Or i can rename everywhere

Copy link
Contributor

Choose a reason for hiding this comment

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

I think similar to isFoo, hasFoo is more indicative of boolean than withFoo. It's okay for now though.

return this.directories.length === 0 && this.files.length === 0;
}

get withFiles(): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

hasFiles?

return this.path === '';
}

get withParent(): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

hasParent?


rename(newName: string): void {
if (this.isLocal === false) {
if (this.parent?.isModel) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use this.hasParent()/this.withParent()

Copy link
Member Author

Choose a reason for hiding this comment

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

so even if I guard with this.hasParent()/this.withParent() typescript apparently still think parent can be undefined
Screenshot 2023-02-09 at 4 27 14 PM

return this.files.length > 0;
}

get withDirectories(): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think similar to isFoo, hasFoo is more indicative of boolean than withFoo. It's okay for now though.

@mykalmax mykalmax merged commit 7e4a25a into main Feb 10, 2023
@mykalmax mykalmax deleted the max/editor_start_screen branch February 10, 2023 18:52
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.

4 participants