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

Log folder's ID and Label (fixes #3724) #3741

Closed
wants to merge 2 commits into from

Conversation

zaynetro
Copy link
Contributor

Purpose

This pull requests fixes issue #3724. Log entries should print folder's ID and Label.

Testing

Testing wasn't necessary for this change.

@calmh
Copy link
Member

calmh commented Nov 17, 2016

So there are two formatting types in use here, inconsistently, since before.

  • %s which just formats a string normally, and can be surrounded by quotes to quote the string: Printf("\"%s\"", "foo") becomes "foo".
  • %q which prints an escaped string, in quotes. Printf("%q", "foo") becomes "foo", same as the example above. But also, Printf("%q", "fo\"o") becomes "fo\"o" while with the %s formatting above it would print as "fo"o".

So anyway, long story short, I think we should probably use "... folder %q (%s)", label, id" to generate something like ... folder "My folder Label" (abcde-12345). No escaping needed in the format string, will quote the label which is the one most likely to include spaces and words and stuff, and generally looks reasonable to me.

@calmh
Copy link
Member

calmh commented Nov 17, 2016

@st-jenkins add to white list

@AudriusButkevicius
Copy link
Member

AudriusButkevicius commented Nov 17, 2016

To be honest, I think we should just print "%s" % cfg and then add a .String() on folder config returning whatever, in case we change something yet again in the future.

@calmh
Copy link
Member

calmh commented Nov 17, 2016

Actually that's a good idea. I was thinking that there were lots of places where the folder label and ID didn't come via a a FolderConfiguration object but that doesn't really seem to be the case, from the diff. So yeah.

Although I wouldn't use String() for it, but something else like Description() instead, or printing the actual object for testing and debugging gets annoying.

@zaynetro
Copy link
Contributor Author

OK, now I am using "%q (%s)" format in all log entries. Folder configuration struct has now a method Description() which prints folder's label and ID.

@calmh
Copy link
Member

calmh commented Nov 21, 2016

Neat! Looks good to me, thanks! There is a merge conflict on the authors change (one of the reasons I prefer doing it), so I'll take care of merging this manually.

calmh pushed a commit that referenced this pull request Nov 21, 2016
@calmh
Copy link
Member

calmh commented Nov 21, 2016

Merged in d3a251e, thanks for the cleanup!

@calmh calmh closed this Nov 21, 2016
@zaynetro
Copy link
Contributor Author

Thanks for the quick replies!

startSequence = 0
continue
}

l.Debugf("Device %v folder %q is delta index compatible (mlv=%d)", deviceID, folder.ID, dev.MaxSequence)
l.Debugf("Device %v folder %q (%s) is delta index compatible (mlv=%d)", deviceID, folder.Label, folder.ID, dev.MaxSequence)
Copy link
Member

Choose a reason for hiding this comment

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

I was saying (over the phone, not sure if it got delivered), that in protocol/bep_extensions.go, we could add:

func (f Folder) Description() string {
    return fmt.Sprintf("%q (%s)", f.Label, f.ID)
}

and use folder.Description() in the remaining cases, that cover the protobuf generated messages.

Copy link
Member

Choose a reason for hiding this comment

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

Done!

@zaynetro zaynetro changed the title WIP: Log folder's ID and Label (fixes #3724) Log folder's ID and Label (fixes #3724) Nov 25, 2016
@zaynetro zaynetro deleted the fix-3732 branch November 25, 2016 00:29
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Nov 22, 2017
@syncthing syncthing locked and limited conversation to collaborators Nov 22, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants