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 config options for Serializer and Formater #74

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alelopezperez
Copy link

Added two config options for quoting table and column name in both the Serializer and Formatter; and change the corresponding code to respect the configs.

Solves issue 1 from #69


if self.quoted_table_names {
self.ident_str(&table.name)?;
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't see anything else. Would no table name be included at all in the fallback case?

Copy link
Author

Choose a reason for hiding this comment

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

You are completely right, i did not put the else case so that would be an error

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. What do you think of creating methods for table_name and column_name that encapsulate the quote / no quote condition in one place? Would that work? I saw some spots where it seemed like the else case was left out. Was that intentional?

@alelopezperez
Copy link
Author

So what I'm thinking is that I could make a function that get called instead of write!(self.dst, "\"{}\"", ...)?; and that function would be a method of the formatter; also it should run on the ident() function. Do you think this approach would be good?

Also thanks for responding, is my first time getting involved in a project of this scale!

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