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

Added wrapper #21

Closed
wants to merge 9 commits into from
Closed

Added wrapper #21

wants to merge 9 commits into from

Conversation

saisandhya01
Copy link
Contributor

I have added the wrapper function. do check it. Also I have checked the working of the program, there were no errors

// baseURL: "https://spider.nitt.edu/bonafide/",
// baseURL: "http://localhost:3001/",
baseURL: "http://localhost:3001/",
Copy link
Member

Choose a reason for hiding this comment

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

We can remove this line. Dont include this in the PR

.json(500)
.json({ message: "Some problem with the file upload" });
// fs.unlinkSync(final_dest);
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

I understand the purpose of the function, but it feels too much hardcoded. If I want to add a column name, or add a new model, I have to change the code at a lot of places. You can optimize the function a bit, in two ways:

  1. See if it is possible to pass the model object itself as the argument, instead of modelName. Explore if there are any getter functions that can retreive all the attributes of the model. Since the row is a sequelize row, the model from which the row was extracted must be present in the row itself. So ideally, you need not even get the model object.
  2. Another way is to let the user specify what columns they want. Something like,
    const row = await database.CertificateType.findAll({})
    let {applier_roll, status} = my_wrapper(row, ['applier_roll', 'status'])
    This array of arguments can also be received from the row model itself actually. Try exploring the attributes of row object and how you can exploit them.

}
});
return object;
}

Copy link
Member

Choose a reason for hiding this comment

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

You can define the function once in the helper.js file and import the function whever it is needed. No need to define it everywhere.

@saisandhya01
Copy link
Contributor Author

While rebasing, I got merge conflicts, that's why I guess there are more commits. The last commit is '0322e93'.
If this is wrong, I will fix this soon.

@gigatesseract
Copy link
Member

gigatesseract commented Oct 1, 2020

Resolve all merge conflicts in your branch and then initiate the PR.
Make sure that your branch is rebased with latest HEAD

@saisandhya01
Copy link
Contributor Author

I have already resolved the merge conflicts in the latest commit.

Fix: Removed hardcoding of attributes in wrapper

Removed repetition of code
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.

A wrapper for database field retreival out of the sequelize object
5 participants