Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Rename generate_solution_type! such that it shows exactly the types #8444

Closed
kianenigma opened this issue Mar 24, 2021 · 3 comments · Fixed by #8447
Closed

Rename generate_solution_type! such that it shows exactly the types #8444

kianenigma opened this issue Mar 24, 2021 · 3 comments · Fixed by #8447
Assignees
Labels
I7-refactor Code needs refactoring. Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be.

Comments

@kianenigma
Copy link
Contributor

This is an example of a solution type generation:

sp_npos_elections::generate_solution_type!(
	#[compact]
	pub struct NposSolution16::<u32, u16, sp_runtime::PerU16>(16)
	// -------------------- ^^ <NominatorIndex, ValidatorIndex, Accuracy>
);

The fact that I have to document it, and double check it is exactly showing the issue here. the syntax of the macro should be such that it clearly shows these. After all, the entire :: and <_> is fake and I am manually parsing it. Maybe something like

sp_npos_elections::generate_solution_type!(
	#[compact]
	pub struct NposSolution16::<Voter = u32, Target = u16, Accuracy = sp_runtime::PerU16>(16)
);
@kianenigma kianenigma added I7-refactor Code needs refactoring. Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be. Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder labels Mar 24, 2021
@kianenigma
Copy link
Contributor Author

@coriolinus unassign if not suitable, I think it is a good way to get familiar with generate_solution_type a wee bit more.

@coriolinus
Copy link
Contributor

Sure, it seems suitable. The proposed syntax seems good to me.

It's worth noting that last week there was a separate discussion (which I'm having trouble finding now) in which similar keyword-style generic arguments were proposed, and some people felt pretty strongly that they were confusingly different from standard Rust syntax. I'm not convinced that that's a strong reason not to use this syntax, but the PR will need to ensure that the macro's documentation contains both a description of what's going on and an example.

@kianenigma
Copy link
Contributor Author

The syntax here is pretty arbitrary and we can parse anything. Also, this will not be used extensivelty, so it need not be "valid rust". Important thing is to make sure while you are writing it, you won't mix VoterIndex and TargetIndex, which is a deadly flaw.

coriolinus added a commit that referenced this issue Mar 24, 2021
Closes #8444.

Just changes the parsing logic for that macro; does not change any
emitted code. The associated types associated with the macro now
require explicit, keyword-style declaration.

**Old**:

```rust
sp_npos_elections::generate_solution_type!(
	#[compact]
	pub struct TestCompact::<VoterIndex, TargetIndex, PerU16>(16)
);
```

**New**:

```rust
sp_npos_elections::generate_solution_type!(
	#[compact]
	pub struct TestCompact::<VoterIndex = VoterIndex, CandidateIndex = TargetIndex, Accuracy = PerU16>(16)
);
```
@ghost ghost closed this as completed in #8447 Mar 28, 2021
ghost pushed a commit that referenced this issue Mar 28, 2021
* make types within `generate_solution_type` macro explicit

Closes #8444.

Just changes the parsing logic for that macro; does not change any
emitted code. The associated types associated with the macro now
require explicit, keyword-style declaration.

**Old**:

```rust
sp_npos_elections::generate_solution_type!(
	#[compact]
	pub struct TestCompact::<VoterIndex, TargetIndex, PerU16>(16)
);
```

**New**:

```rust
sp_npos_elections::generate_solution_type!(
	#[compact]
	pub struct TestCompact::<VoterIndex = VoterIndex, CandidateIndex = TargetIndex, Accuracy = PerU16>(16)
);
```

* un-ignore doc-tests

* use new form in bin/node/runtime/

* rename CandidateIndex -> TargetIndex

* add tests demonstrating some potential compile failures
hirschenberger pushed a commit to hirschenberger/substrate that referenced this issue Apr 14, 2021
…#8447)

* make types within `generate_solution_type` macro explicit

Closes paritytech#8444.

Just changes the parsing logic for that macro; does not change any
emitted code. The associated types associated with the macro now
require explicit, keyword-style declaration.

**Old**:

```rust
sp_npos_elections::generate_solution_type!(
	#[compact]
	pub struct TestCompact::<VoterIndex, TargetIndex, PerU16>(16)
);
```

**New**:

```rust
sp_npos_elections::generate_solution_type!(
	#[compact]
	pub struct TestCompact::<VoterIndex = VoterIndex, CandidateIndex = TargetIndex, Accuracy = PerU16>(16)
);
```

* un-ignore doc-tests

* use new form in bin/node/runtime/

* rename CandidateIndex -> TargetIndex

* add tests demonstrating some potential compile failures
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I7-refactor Code needs refactoring. Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants