Skip to content

Fix: Format model on save#1309

Merged
mykalmax merged 6 commits intomainfrom
fix-format-model-on-save
Aug 16, 2023
Merged

Fix: Format model on save#1309
mykalmax merged 6 commits intomainfrom
fix-format-model-on-save

Conversation

@mykalmax
Copy link
Contributor

No description provided.

Comment on lines 67 to 73
Copy link
Contributor

Choose a reason for hiding this comment

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

What about formatting content instead of the file? Also, do we want to format when renaming a file? Users might not expect that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Settings wouldn't have changed, right?

Suggested change
path_or_new_path_mapping = await get_path_mapping(settings=get_settings())
path_or_new_path_mapping = await get_path_mapping(settings=settings)

@mykalmax mykalmax force-pushed the fix-format-model-on-save branch from c7be92b to 56789db Compare August 15, 2023 18:15
@mykalmax
Copy link
Contributor Author

mykalmax commented Aug 15, 2023

@vchan
In general i would expect some formatting on save. But in our case formatting models not just fix indentation.
Before:
Screenshot 2023-08-15 at 11 11 05 AM
After:
Screenshot 2023-08-15 at 11 10 59 AM

@mykalmax mykalmax force-pushed the fix-format-model-on-save branch from 56789db to 8e56565 Compare August 15, 2023 19:52
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use the model's dialect if it has a non-default dialect?

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the file isn't a sql file?

@mykalmax mykalmax force-pushed the fix-format-model-on-save branch from 78b95ce to 7619f80 Compare August 15, 2023 21:15
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the source code and docstring for parse(), default_dialect should be the project wide config dialect.

Comment on lines 86 to 93
Copy link
Contributor

Choose a reason for hiding this comment

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

If the formatting fails, should we still write the original content?

Comment on lines 69 to 76
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can update get_path_mapping to map paths to models and use it here instead of searching through all models every time user saves.

Copy link
Contributor Author

@mykalmax mykalmax Aug 16, 2023

Choose a reason for hiding this comment

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

we're using get_path_mapping not just for models but for files in general

@mykalmax mykalmax force-pushed the fix-format-model-on-save branch from 7619f80 to 5ebcfed Compare August 16, 2023 16:19
Comment on lines 27 to 34
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this removed? Is it because the file watcher reloads the context on file changes now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, file watcher reloads the context on model or seed file changes and collecting models to send over SSE

Comment on lines 84 to 93
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed or replaced with a SSE message warning the file couldn't be formatted. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Lines 75-78 can all go into this if block.

@mykalmax mykalmax force-pushed the fix-format-model-on-save branch from d03e309 to 69c63a4 Compare August 16, 2023 19:11
@mykalmax mykalmax merged commit 77d684b into main Aug 16, 2023
@mykalmax mykalmax deleted the fix-format-model-on-save branch August 16, 2023 19:51
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.

2 participants