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

review structure #24

Open
wants to merge 1 commit into
base: solutions
Choose a base branch
from

Conversation

MrishoLukamba
Copy link

Check the structure

  1. I have refactored each functionality and later merged all the functionalities.
  2. The external data is just openFDA data for health.
  3. If the structure is okey, I can proceed to exercise .
  4. @tdelabro

Copy link
Collaborator

@tdelabro tdelabro left a comment

Choose a reason for hiding this comment

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

everything under you .idea should not be committed.

"disclaimer": "Do not rely on openFDA to make decisions regarding medical care. While we make every effort to ensure that data is accurate, you should assume all results are unvalidated. We may limit or otherwise restrict your access to the API in line with our Terms of Service.",
"terms": "https://open.fda.gov/terms/",
"license": "https://open.fda.gov/license/",
"last_updated": "2022-08-31",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test will fail as soon as they update their database again.
Not sustainable.
You should think about checking something else. Or that the result of get_external_data is more than 22839



impl<T: Config> Pallet<T> {
pub fn get_external_data() -> Result<u32, http::Error> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use more experessive function name.
This one get the total number of FDA enforcement. Call in so it express this


}

pub fn parse_to_int(body: JsonValue) -> Option<u32>{
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function does not parse_to_int. It extracts the total number of enforcement from the body. Change the name so it expresses this


}

pub fn parse_to_int(body: JsonValue) -> Option<u32>{
Copy link
Collaborator

Choose a reason for hiding this comment

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

this function does not need to be in impl<T: Config> Pallet<T> {}.
It is not runtime dependant and can live outside of this code block

"derive",
] }
scale-info = { version = "2.1.1", default-features = false, features = ["derive"] }
frame-benchmarking = { version = "4.0.0-dev", default-features = false, optional = true, git = "https://github.com/paritytech/substrate.git", branch = "polkadot-v0.9.28" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

In our tutorials we are using substrate on branch polkadot-v0.9.22.
You should use the same one

@@ -0,0 +1,22 @@

Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this module do?

);


impl frame_system::Config for TestRuntime {
Copy link
Collaborator

Choose a reason for hiding this comment

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

split your mocking code in a mock file


impl<T: Config> Pallet<T>{

pub fn send_signed_transaction()-> Result<(),&'static str> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function feed the chain with an offchain value.
The way it does it is by sending a signed transaction.
Name it based on what it does not how it does it. It will be way easier for students to understand

@@ -0,0 +1,88 @@
#![cfg_attr(not(feature = "std"),no_std)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand what this new section does.
It doesn't add much to what has already been demonstrated in the previous section

#[pallet::hooks]
impl<T:Config> Hooks<BlockNumberFor<T>> for Pallet<T>{
fn offchain_worker(_n: BlockNumberFor<T>) {
Self::send_signed_transaction().map_err(|_|log::info!("Failed Function"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

When is this one actually called?
Who decides to execute this one?

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