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

clang-format #3

Merged
merged 1 commit into from
May 27, 2020
Merged

Conversation

daschuer
Copy link

No description provided.

Copy link
Owner

@xeruf xeruf left a comment

Choose a reason for hiding this comment

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

I'm not contempt with the current formatting configuration

Comment on lines +148 to +154
" COUNT(case library.mixxx_deleted when 0 then 1 else null end) "
"AS count, "
" SUM(case library.mixxx_deleted when 0 then library.duration "
"else 0 end) AS durationSeconds "
"FROM Playlists "
"LEFT JOIN PlaylistTracks ON PlaylistTracks.playlist_id = Playlists.id "
"LEFT JOIN PlaylistTracks ON PlaylistTracks.playlist_id = "
"Playlists.id "
Copy link
Owner

Choose a reason for hiding this comment

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

These linebreaks illustrate why I am against automatically hard-wrapping

src/library/trackset/crate/cratestorage.cpp Show resolved Hide resolved
Comment on lines +391 to +397
auto newName = QInputDialog::getText(nullptr,
tr("Rename Crate"),
tr("Enter new name for crate:"),
QLineEdit::Normal,
oldName,
&ok)
.trimmed();
Copy link
Owner

Choose a reason for hiding this comment

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

this looks weird

Copy link
Author

Choose a reason for hiding this comment

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

Yes, right. If you like you can check if you can trick clang-format here.

Copy link
Owner

Choose a reason for hiding this comment

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

have adjusted this, but I think the formatter should never produce something like this.

Comment on lines +361 to +362
CrateId crateId = CrateFeatureHelper(m_pTrackCollection,
m_pConfig)
Copy link
Owner

Choose a reason for hiding this comment

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

meh

Copy link
Owner

Choose a reason for hiding this comment

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

have adjusted this, but I think the formatter should never produce something like this.

src/library/trackset/crate/cratefeature.cpp Show resolved Hide resolved
&LibraryFeature::loadTrack,
this,
&Library::slotLoadTrack);
connect(feature, &LibraryFeature::loadTrack, this, &Library::slotLoadTrack);
Copy link
Owner

Choose a reason for hiding this comment

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

this is an unexpected inconsistency with the code above and below

Copy link
Author

Choose a reason for hiding this comment

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

The good thing with our two phase clang-format pre-commit hook is that you can help here.
Clang format will not unbreakable the lines if you break after each parameter.
So if this is itching you, please break this statement manually.

Comment on lines 482 to +486
QStringLiteral("SELECT *, "
"(SELECT COUNT(*) FROM %1 WHERE %2.%3 = %1.%4 and %1.%5 in (%9)) AS %6, "
"(SELECT COUNT(*) FROM %1 WHERE %2.%3 = %1.%4 and "
"%1.%5 in (%9)) AS %6, "
"0 as %7 FROM %2 ORDER BY %8")
.arg(
CRATE_TRACKS_TABLE,
.arg(CRATE_TRACKS_TABLE,
Copy link
Owner

Choose a reason for hiding this comment

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

I manually formatted this part and tried to find a balance between length and readability, now the formatter breaks it at some arbitrary point to fit the length

Copy link
Author

Choose a reason for hiding this comment

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

The original line was 101. I have no idea why clang-format aligns with (. We don't want that.
You may try to manually break after ( and restore the original version.

Comment on lines +101 to +105
CapabilitiesFlags caps = TRACKMODELCAPS_RECEIVEDROPS |
TRACKMODELCAPS_ADDTOPLAYLIST | TRACKMODELCAPS_ADDTOCRATE |
TRACKMODELCAPS_ADDTOAUTODJ | TRACKMODELCAPS_EDITMETADATA |
TRACKMODELCAPS_LOADTODECK | TRACKMODELCAPS_LOADTOSAMPLER |
TRACKMODELCAPS_LOADTOPREVIEWDECK | TRACKMODELCAPS_REMOVE_CRATE |
Copy link
Owner

Choose a reason for hiding this comment

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

this was much more readable when each flag had its own line

src/library/trackset/playlistfeature.cpp Show resolved Hide resolved
src/library/trackset/setlogfeature.cpp Show resolved Hide resolved
@daschuer
Copy link
Author

First of all, this PR is just a service for you to get around the pending issues with the pre-commit hook.

It is what the pre-commit hook should have been done automatically. I have no Idea why, that has failed for you. Do you.

This PR just reflects the agreed .clang-format, so there is no point for discussion, to get the target PR merge.

This does not mean that the configuration cannot be improved. I will now respond to your inline comments.

@daschuer daschuer closed this May 23, 2020
@daschuer daschuer reopened this May 23, 2020
@daschuer
Copy link
Author

I have accidentally hit the "Close and comment" button.

@xeruf xeruf merged commit 4f032f7 into xeruf:refactor-basetrackset May 27, 2020
@xeruf
Copy link
Owner

xeruf commented May 30, 2020

I have adjusted a few things, but I would like to have the format options revisited to resolve my remaining comments automatically

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