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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

minor fix: bool should not be empty when there is not null constraint #1711

Merged
merged 5 commits into from
Sep 29, 2023
Merged

minor fix: bool should not be empty when there is not null constraint #1711

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6775caf
Fix bool option
azmy60 Sep 28, 2023
89931e8
Map boolean to each dialect
azmy60 Sep 28, 2023
13fcd3c
refactor
azmy60 Sep 28, 2023
43deb16
revert some files
azmy60 Sep 28, 2023
e1476a9
simplify values
azmy60 Sep 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 21 additions & 3 deletions apps/studio/src/components/tableview/TableTable.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,15 +646,33 @@ export default Vue.extend({
editorParams: {
verticalNavigation: useVerticalNavigation ? 'editor' : undefined,
search: true,
values: /^(bool|boolean)$/i.test(column.dataType)
? [true, false]
: undefined,
allowEmpty: true,
// elementAttributes: {
// maxLength: column.columnLength // TODO
// }
},
}

if (/^(bool|boolean)$/i.test(column.dataType)) {
// BigInt to match the value from backend
const values = [
{ label: '0', value: BigInt(0) },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't labels here be true and false? For non-Sqlite boolean columns especially.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, for non-sqlite, shouldn't the values be 'true' and 'false' also?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, sorry I missed the dropdown will show false/true section below.

Question though -- do 0 and 1 map to boolean data types in all the other databases? Maybe that needs a test to confirm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I thought boolean is only in sqlite. 0 and 1 do not map to boolean types in postgres. With that I think it's simpler to use false / true like before. It's easy to switch. Give me a second.

Copy link
Contributor Author

@azmy60 azmy60 Sep 28, 2023 •
edited

Choose a reason for hiding this comment

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

Actually, it needs some generic mapping like mysqlDialect.booleanValues: { "true": BigInt(1), "false": "BigInt(0)" }, and then do this to each dialect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only thing that changes here is in sqlite, we're showing 0 and 1 in dropdown, instead of false and true. That's probably the simplest solution we could have. Is that ok?

The problem with sqlite is that boolean is represented as 0 and 1, and when using false and true as the dropdown options, it doesn't match up, which then shows that (empty) value.

We still can use the false and true as values for sqlite though if you want. I just need some time to wire up the good solution to that.

{ label: '1', value: BigInt(1) },
]
if (column.nullable) values.push({ label: '(NULL)', value: null })
result.editorParams['values'] = values
result.editorParams['itemFormatter'] = (
label: string,
value: BigInt | null
) => {
// This will make sure that the dropdown will show false/true.
// We can't just set the label to false/true initially because
// that will change the cell when editing, which can be confusing.
if (value !== null) return Boolean(value).toString()
return label
}
}

results.push(result)

if (keyDatas && keyDatas.length > 0) {
Expand Down