Remove some duplicated types in RPC #115
Conversation
| #[cfg_attr(feature = "std", derive(Debug))] | ||
| #[cfg_attr(feature = "std", derive(Serialize, Deserialize))] | ||
| #[cfg_attr(feature = "std", serde(rename_all = "camelCase"))] | ||
| pub struct FarmerMetadata { |
There was a problem hiding this comment.
I prefer it FarmerConfig or something as metadata is sort of a special word for me.
| pub struct FarmerMetadata { | |
| pub struct FarmerConfig { |
There was a problem hiding this comment.
Arguably this is closer to the core primitives, but not really. Another data structure that kind of belongs to a separate crate altogether.
It is called metadata because it is not a static configuration, but rather something that is derived from the blockchain itself. Maybe metadata is not the best word, but I don't feel like this is a configuration either.
There was a problem hiding this comment.
How about FarmerProperty? Also not good at naming though :D ...
| #[cfg_attr(feature = "std", derive(Debug))] | ||
| #[cfg_attr(feature = "std", derive(Serialize, Deserialize))] | ||
| #[cfg_attr(feature = "std", serde(rename_all = "camelCase"))] | ||
| pub struct FarmerMetadata { |
There was a problem hiding this comment.
Arguably this is closer to the core primitives, but not really. Another data structure that kind of belongs to a separate crate altogether.
It is called metadata because it is not a static configuration, but rather something that is derived from the blockchain itself. Maybe metadata is not the best word, but I don't feel like this is a configuration either.
Let's do that as I don't see any other solution at present. |
liuchengxu
force-pushed
the
duplicated-types
branch
from
d01fcd2
to
970e96b
Compare
liuchengxu
force-pushed
the
duplicated-types
branch
from
970e96b
to
48e3bba
Compare
Also add a LICENSE file under the crate.
`subspace_submitSolutionResponse`
Sure, always a fan of squash merge and I still think we should make it the default behavior :D. It's hard (at least for me) to maintain a pretty meaningful commit history per PR, and we'll also definitely have a lot of updates based on the review feedback. If we really want to know about the rationale bebind the code changes, we could go read the discussions under the related PR. |
|
The challenge with using squash merge is that for complex changes it is impossible to understand what was happening after the fact from the main branch and going to PRs for every commit is also not very productive, it is much nicer to be able to use git directly to see the whole meaningful history. I think it is extremely important to see why things were changed, what was the thought process and how things were moved around instead of just seeing 2 snapshots with a lot of invisible intermediate steps in between. For instance see https://github.com/subspace/subspace/pull/91/commits, there are just 2 commits: one moves code without meaningful changes (so you don't need to review it) and another implements what needed to be implemented. Initially it was ~8 commits where code was copied in modified form first, then changed a few times and then old version was replaced with a call to a new one. I think it is obvious what is easier to read and review, both in PR and in the main branch. If we squash those 2 commits then you can no longer skip reviewing both old and new code because you can't quickly distinguish what has changed and what hasn't. It also helps with So while it is fine to squash merge simpler changes, for big changes it is extremely important IMHO to have useful history for prosperity and useful commits in the PR itself to save reviewer's time. Don't get me wrong, I don't always create perfect commits right away either, but I try to put things into a better shape when PR time comes, such that squash merge is not needed as the result. |
|
For the huge non-trivial code changes, we could split it into multiple meaningful PRs instead of maintaining multiple commits in one PR. I agree reading the PRs is probably less productive in some cases, but on the other hand, I believe it can convey more background by the discussions compared to using git directly. The nice small diff could tell a lot, but won't tell everything. I'm not sure how you see the commit history of subspace and substrate, for me, the commit history of substrate gives much more clean useful information when I skim the commit history. Just a thought, I can live with the current merge process :D. |
|
No offense to anyone, but when I see this from maintainers I want to cry: https://github.com/paritytech/substrate/pull/9394/commits and typically it gets merged with the same kinds of "useful" facepalms: paritytech/substrate@1d5abf0 So not everything other people do should necessarily be replicated. |
|
Well, I'll ignore the other commit messages and they will be folded when being squash merge, the very first commit message that will be retained and displayed just makes sense and implies the meat of that PR. 🙈 |
I make it a draft PR because I'm unsure about some leftover on the duplicated types that unavoidably pull in some types from the substrate, e.g.,
public_keyinSolution,slotinNewSlotInfo. Should be reviewed per commit.Close #103