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

Update comment on the schema_filter key #1165

Merged
merged 1 commit into from May 21, 2020

Conversation

Gwalchmei
Copy link
Contributor

See doctrine/dbal#3986

The schema_filter key does not filter only tables.

@@ -95,7 +95,7 @@ Configuration Reference
platform_service: ~
auto_commit: ~

# If set to "/^sf2_/" all tables not prefixed with "sf2_" will be ignored by the schema
# If set to "/^sf2_/" all tables, and any named objects such as sequences, not prefixed with "sf2_" will be ignored by the schema
Copy link
Member

Choose a reason for hiding this comment

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

Please wrap the comment consistently with the previous version

Copy link
Member

Choose a reason for hiding this comment

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

WDYM? In both versions it's single line so no wrapping, isn't it?

Copy link
Member

@greg0ire greg0ire May 8, 2020

Choose a reason for hiding this comment

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

I was not talking about the previous version of the line, but about the previous version of the sentence, and the end of the sentence is on a new line (tool. is on line 99), so there is wrapping. The goal here is to avoid horizontal in the docs, let's not make the docs harder to read.

@@ -95,8 +95,8 @@ Configuration Reference
platform_service: ~
auto_commit: ~

# If set to "/^sf2_/" all tables, and any named objects such as sequences, not prefixed with "sf2_" will be ignored by the schema
# tool. This is for custom tables which should not be altered automatically.
# If set to "/^sf2_/" all tables, and any named objects such as sequences, not prefixed with "sf2_" will be ignored by the schema tool.
Copy link
Member

Choose a reason for hiding this comment

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

What I mean by wrapping is avoiding long lines by splitting them when reaching a certain length

Copy link
Member

@ostrolucky ostrolucky left a comment

Choose a reason for hiding this comment

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

Current state of this patch is fine by me. This is not a longest line we have here, check https://github.com/doctrine/DoctrineBundle/pull/1165/files#diff-93d26a68e69554f4bf0e01c6245dc390L495. Can't really imagine how would we split this to more lines, split sentence at random point? I don't see this done anywhere else in this document.

@greg0ire
Copy link
Member

Can't really imagine how would we split this to more lines, split sentence at random point?

Either that or you can use https://sembr.org/ if you want. My point is, this is going to look really bad here:
Peek 2020-05-21 13-25

You should not have to scroll horizontally here, one dimension to explore should be enough.

@greg0ire
Copy link
Member

greg0ire commented May 21, 2020

But it is currently split, look:

# If set to "/^sf2_/" all tables not prefixed with "sf2_" will be ignored by the schema
# tool. This is for custom tables which should not be altered automatically.
schema_filter: ~

Besides, as you shown in GIF users have to scroll already, so I don't see what this improves, users will still have to scroll.

IIRC someone (Javier Eguiluz?) asked how we could improve this, and we didn't find a solution, but let's not make it worse, ok? If people just need to have a look at the docs for schema_filter, which might very well happen, let's not make these people scroll please.

EDIT: ah I was confusing with this somewhat related, but not completely discussion

See doctrine/dbal#3986

The `schema_filter` key does not filter only tables.

Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>
@ostrolucky ostrolucky changed the base branch from master to 1.12.x May 21, 2020 11:48
@ostrolucky ostrolucky merged commit f768b8f into doctrine:1.12.x May 21, 2020
@alcaeus alcaeus added this to the 1.12.9 milestone May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants