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

DOC-12385: Copy across missing snippets #891

Open
wants to merge 5 commits into
base: release/3.1
Choose a base branch
from

Conversation

ElliotFrancisHunter
Copy link
Collaborator

Move snippets from p2psync-websocket.cs to Program.cs

Move snippets from p2psync-websocket.cs to Program.cs
@borrrden
Copy link
Member

This has broken compilation and cannot be merged

Copy link
Member

@borrrden borrrden left a comment

Choose a reason for hiding this comment

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

This needs to be written in a way that matches the existing code in Program.cs. It cannot just be plopped in. This is what the GH checks are meant to prevent. These snippets should be able to be compiled.

@osfameron
Copy link
Collaborator

Thanks @borrrden - who of the devs can we ask to look at that?
Can you let us know which ticket type to create and link to the DOC ticket above?

@borrrden
Copy link
Member

Ideally it would be the person who wrote them that puts them in correctly. Otherwise it's going to be me. Are all of these snippets needed? Some of them are just one line which seems a bit excessive to create an entire self contained snippet for.

Copy link
Member

@borrrden borrrden left a comment

Choose a reason for hiding this comment

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

Would like some clarification here on whether these are all important. There are a lot of tags in here including "how to call start" which seems pretty obvious, does it really need a snippet?

Anyway perhaps a description of what each one wants to do is in order because some of them are suspicious. I'll put in comments inline.

// . . . preceding code. for example . . .
private static ListenerToken _thisListenerToken;
var Database thisDB;
// . . . other code . . .
Copy link
Member

Choose a reason for hiding this comment

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

What is this meant to represent?

// tag::p2p-act-rep-config-cont[]
// Configure Sync Mode
thisConfig.Continuous = true; // default value
Copy link
Member

Choose a reason for hiding this comment

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

This is not the default for this property

// tag::p2p-act-rep-start[]
// Start replicator
thisReplicator.Start(); // <.>
Copy link
Member

Choose a reason for hiding this comment

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

This is the same as starting any other replicator...does it really need a separate snippet?

// end::p2p-act-rep-config-cacert[]

#warning p2p-act-rep-config-cacert-pinned used, but contains nothing
// tag::p2p-act-rep-config-cacert-pinned[]
// Only CA Certs accepted
Copy link
Member

Choose a reason for hiding this comment

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

Misleading comment here. Only certificates issued by this CA are accepted, not any CA cert.

while (_thisReplicator.Status.Activity != ReplicatorActivityLevel.Stopped) {
// Database cannot close until replicators are stopped
Console.WriteLine($"Waiting for replicator to stop (currently {_thisReplicator.Status.Activity})...");
Thread.Sleep(200);
Copy link
Member

Choose a reason for hiding this comment

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

This is quite domain specific and unlikely to make a helpful snippet

Console.WriteLine($"Waiting for replicator to stop (currently {_thisReplicator.Status.Activity})...");
Thread.Sleep(200);
}
_thisDatabase.Close();
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need to be here?

// tag::p2p-act-rep-stop[]
// Stop replication.
thisReplicator.Stop(); // <.>
Copy link
Member

Choose a reason for hiding this comment

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

Same as any other replication...

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.

3 participants