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

make types within generate_solution_type macro explicit #8447

Merged
7 commits merged into from
Mar 28, 2021
Merged

make types within generate_solution_type macro explicit #8447

7 commits merged into from
Mar 28, 2021

Conversation

coriolinus
Copy link
Contributor

@coriolinus coriolinus commented Mar 24, 2021 •
edited
Loading

Closes #8444.

polkadot companion: paritytech/polkadot#2707

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.

Note: while the new syntax looks like keyword arguments from a language such as Python,
unlike real keyword arguments, the order still matters. Checks in the syntax parser ensure that
the right arguments appear in the right order.

Old:

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

New:

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

@coriolinus
make types within generate_solution_type macro explicit
6e5a8c6 
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)
);
```
@coriolinus coriolinus self-assigned this Mar 24, 2021
@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Mar 24, 2021
@coriolinus coriolinus added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Mar 24, 2021
kianenigma
kianenigma approved these changes Mar 25, 2021
View reviewed changes
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

In the jargon of npos_election stuff, we have voters and targets, let's stick to that and rename CandidateIndex to TargetIndex, otherwise looks good, thanks!

@kianenigma
Copy link
Contributor

Note: while the new syntax looks like keyword arguments from a language such as Python,
unlike real keyword arguments, the order still matters. Checks in the syntax parser ensure that
the right arguments appear in the right order.

Would be good to have a test to make sure

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

fails.

gui1117
gui1117 approved these changes Mar 25, 2021
View reviewed changes
Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

looks good to me

primitives/npos-elections/compact/src/lib.rs Outdated Show resolved Hide resolved
@coriolinus coriolinus mentioned this pull request Mar 25, 2021
Merged
@coriolinus
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Mar 25, 2021

Waiting for commit status.

@ghost
Copy link

ghost commented Mar 25, 2021

Checks failed; merge aborted.

@coriolinus
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Mar 25, 2021

Waiting for commit status.

@ghost
Copy link

ghost commented Mar 25, 2021

Checks failed; merge aborted.

@coriolinus
Copy link
Contributor Author

Looking for recommendations for how to proceed with this. The polkadot companion stage fails because that PR is not mergeable. The only reason I can see that paritytech/polkadot#2707 is not mergeable is that its gitlab-test-linux-stable CI check fails.

I can replicate the failure in that CI check only if I do not have the ~/.cargo/config paths set appropriately for a companion. When it's set as required, then everything builds as expected locally.

@gui1117
Copy link
Contributor

gui1117 commented Mar 25, 2021

did you merge polkadot master into the companion ?

@bkchr
Copy link
Member

bkchr commented Mar 25, 2021

The companion wasn't approved.

@kianenigma
Copy link
Contributor

I can replicate the failure in that CI check only if I do not have the ~/.cargo/config paths set appropriately for a companion.

The Way that polkadot companions are merged is that their own CI is supposed to fail, so what you are seeing is correct. For a companion, you check the build status in the substrate polkadot-build CI job.

Once the substrate PR is merged, the bot does a cargo update -p sp-io in polkadot, then run again.

@kianenigma
Copy link
Contributor

bot merge

@ghost
Copy link

ghost commented Mar 28, 2021

Trying merge.

@ghost ghost merged commit 505a8d6 into master Mar 28, 2021
@ghost ghost deleted the prgn-generate-solution-keyword-types branch March 28, 2021 08:21
ordian added a commit that referenced this pull request Mar 31, 2021
@ordian
Merge branch 'master' into ao-update-wasmtime
ded49c0 
* master: (84 commits)
  Duplicate logging to stdout (#8495)
  Fix sync restart (#8497)
  client: fix justifications migration (#8489)
  helper macro to create storage types on the fly (#8456)
  Make `BlockImport` and `Verifier` async (#8472)
  Get rid of `test-helpers` feature in sc-consensus-babe (#8486)
  Enhancement on Substrate Node Template (#8473)
  Add Social Network (#8065)
  Prepare UI tests for Rust 1.51 & new CI image (#8474)
  Benchmarking pallet-example (#8301)
  Use pathbuf for remote externalities (#8480)
  Bring back the on_finalize weight of staking. (#8463)
  Implement `fungible::*` for Balances (#8454)
  make types within `generate_solution_type` macro explicit (#8447)
  [pallet-staking] Refund unused weight for `payout_stakers` (#8458)
  Use `async_trait` in sc-consensus-slots (#8461)
  Repot frame_support::traits; introduce some new currency stuff (#8435)
  Fix &mut self -> &self in add_known_address (#8468)
  Add NetworkService::add_known_address (#8467)
  Fix companion check (#8464)
  ...
hirschenberger pushed a commit to hirschenberger/substrate that referenced this pull request Apr 14, 2021
@coriolinus @hirschenberger
make types within generate_solution_type macro explicit (paritytech…
0de37ac 
…#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 pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kianenigma kianenigma kianenigma approved these changes

@gui1117 gui1117 gui1117 approved these changes

Assignees

@coriolinus coriolinus

Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rename generate_solution_type! such that it shows exactly the types
4 participants
@coriolinus @kianenigma @gui1117 @bkchr