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 csharp namespace option #228

Closed
wants to merge 1 commit into from
Closed

Conversation

Varorbc
Copy link

@Varorbc Varorbc commented Jun 4, 2024

part of #227

@Varorbc Varorbc requested a review from a team as a code owner June 4, 2024 11:28
@Varorbc
Copy link
Author

Varorbc commented Jun 4, 2024

@jt-nti please review

@jt-nti
Copy link
Member

jt-nti commented Jun 4, 2024

At first glance, "Hyperledger.Fabric" seems good to me. (I've opened hyperledger-labs/governance#67 to reserve the "Hyperledger" prefix.)

The specific namespaces I looked at mostly seemed reasonable, although I noticed you skipped "orderer". on a couple- was that intentional?

@Varorbc
Copy link
Author

Varorbc commented Jun 4, 2024

My negligence, I have fixed it.

@Varorbc
Copy link
Author

Varorbc commented Jun 5, 2024

The build failed. What should I do?

@jt-nti
Copy link
Member

jt-nti commented Jun 6, 2024

@Varorbc you shouldn't need to do anything about the build failure- it's picking up the incompatible change to the namespace but since C# is new as far as Fabric protos go, I think that's ok. Once the namespaces have been added though, it would be good to avoid any future breaking changes so it's definitely worth getting them right first time.

I've been playing with adding C# bindings and these are the namespaces that came out using the Hyperledger.Fabric.Protos prefix you proposed, so it would be good if the manual csharp_namespace options matched:

$ ls -R
Hyperledger

./Hyperledger:
Fabric

./Hyperledger/Fabric:
Protos

./Hyperledger/Fabric/Protos:
Common          Etcdraft        Google          Kvrwset         Msp             Protos          Rwset
Discovery       Gateway         Gossip          Lifecycle       Orderer         Queryresult     Transientstore

./Hyperledger/Fabric/Protos/Common:
Collection.cs           Configtx.cs             Ledger.cs               Policies.cs
Common.cs               Configuration.cs        MspPrincipal.cs

./Hyperledger/Fabric/Protos/Discovery:
Protocol.cs     ProtocolGrpc.cs

./Hyperledger/Fabric/Protos/Etcdraft:
Configuration.cs        Metadata.cs

./Hyperledger/Fabric/Protos/Gateway:
Gateway.cs      GatewayGrpc.cs

./Hyperledger/Fabric/Protos/Google:
Rpc

./Hyperledger/Fabric/Protos/Google/Rpc:
Status.cs

./Hyperledger/Fabric/Protos/Gossip:
Message.cs      MessageGrpc.cs

./Hyperledger/Fabric/Protos/Kvrwset:
KvRwset.cs

./Hyperledger/Fabric/Protos/Lifecycle:
ChaincodeDefinition.cs  Db.cs                   Lifecycle.cs

./Hyperledger/Fabric/Protos/Msp:
Identities.cs   MspConfig.cs

./Hyperledger/Fabric/Protos/Orderer:
Ab.cs                   Blockattestation.cs     Cluster.cs              Clusterserver.cs        Configuration.cs
AbGrpc.cs               BlockattestationGrpc.cs ClusterGrpc.cs          ClusterserverGrpc.cs    Smartbft

./Hyperledger/Fabric/Protos/Orderer/Smartbft:
Configuration.cs

./Hyperledger/Fabric/Protos/Protos:
Chaincode.cs            Collection.cs           Peer.cs                 ProposalResponse.cs     Snapshot.cs
ChaincodeEvent.cs       Configuration.cs        PeerGrpc.cs             Query.cs                SnapshotGrpc.cs
ChaincodeShim.cs        Events.cs               Policy.cs               Resources.cs            Transaction.cs
ChaincodeShimGrpc.cs    EventsGrpc.cs           Proposal.cs             SignedCcDepSpec.cs

./Hyperledger/Fabric/Protos/Queryresult:
KvQueryResult.cs

./Hyperledger/Fabric/Protos/Rwset:
Rwset.cs

./Hyperledger/Fabric/Protos/Transientstore:
Transientstore.cs

@Varorbc
Copy link
Author

Varorbc commented Jun 6, 2024

Do you mean that I changed the namespace of KvQueryResult to thisHyperledger.Fabric.Protos.Queryresult.KvQueryResult.cs
R in the word QueryResult is lowercase instead of uppercase?

@jt-nti
Copy link
Member

jt-nti commented Jun 6, 2024

@Varorbc which do you think is more natural for a C# developer? I can add overrides if you think it would be better to adjust some of the defaults, e.g.

    - file_option: csharp_namespace
      path: ledger/queryresult/kv_query_result.proto
      value: Hyperledger.Fabric.Protos.QueryResult

@Varorbc
Copy link
Author

Varorbc commented Jun 6, 2024

@jt-nti This is a naming convention, and C# developers prefer PascalCase style.

@jt-nti
Copy link
Member

jt-nti commented Jun 7, 2024

@Varorbc that makes sense. There also seems to be a convention to only uppercase two letter acronyms, so I think MSP should be Msp. (And similarly SmartBFT should be SmartBft in #229)

@Varorbc
Copy link
Author

Varorbc commented Jun 8, 2024

@jt-nti I think it's all right whether the abbreviation of a phrase is capitalized or capitalized, depending on which one you think is better, but there are some special cases, such as RSA, which use all capitals instead of Rsa.

@Varorbc
Copy link
Author

Varorbc commented Jun 8, 2024

I see that MSPConfig is used here, not MspConfig.

I see that SmartBFT is used here, not SmartBft.

@jt-nti
Copy link
Member

jt-nti commented Jun 10, 2024

@Varorbc the actual message names in the proto files don't follow the C# conventions and I don't know of any way to customise them in code generation, but I think it probably makes sense to follow the acronym conventions for the new namespaces.

https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/capitalization-conventions?redirectedfrom=MSDN

@Varorbc
Copy link
Author

Varorbc commented Jun 10, 2024

Yes, it seems that there is no way to customize the message name.

@Varorbc Varorbc closed this Sep 18, 2024
@Varorbc Varorbc deleted the csharp branch September 18, 2024 08:02
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