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

Feat/web api missing functions #69

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

abderrahmaneMustapha
Copy link
Member

@abderrahmaneMustapha abderrahmaneMustapha commented Jun 4, 2021

Description

i added some missing functions , that i need to use in leblad web api

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Updated the dataset

Checklist:

  • I checked that there's no dataset update (can be done by running npm run update-dataset)
  • npm test passes on my machine
  • npm run lint passes on my machine

@codecov-commenter
Copy link

codecov-commenter commented Jun 5, 2021

Codecov Report

Merging #69 (b7b52a9) into develop (5cd401b) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##           develop       #69   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           19        23    +4     
  Lines          164       210   +46     
  Branches        31        38    +7     
=========================================
+ Hits           164       210   +46     
Impacted Files Coverage Δ
.../api/getBaladiyaByCodeAndDairaCodeAndWilayaCode.js 100.00% <100.00%> (ø)
src/api/getBaldiyaByCodeForWilaya.js 100.00% <100.00%> (ø)
...c/api/getBaldiyatsForDairaByCodeForWilayaByCode.js 100.00% <100.00%> (ø)
src/api/getDairaByCodeAndWilayaCode.js 100.00% <100.00%> (ø)
src/index.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5cd401b...b7b52a9. Read the comment docs.

Copy link
Member

@ZibanPirate ZibanPirate 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, these functionally are needed for a web server where you have enpoints such as

.../api/Wilayas/{wilayaCode}/Daira/{dairaCode}/Baladiya/{baladiyaCode}

the code itself looks good.

The only thing missing is updating the readme documentation, can you please do that @abderrahmaneMustapha

@abderrahmaneMustapha
Copy link
Member Author

@ZibanPirate done , i updated the documentation

Copy link
Collaborator

@Fcmam5 Fcmam5 left a comment

Choose a reason for hiding this comment

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

Thanks man for your great efforts! I just got some comments (please don't hate me).

Also, please next time file an issue before started working on such features just to keep track of our changes, aaaand please it would be easier for us to review small PRs as you know, so it'd be ideal to work on each feature on a small PR which should be linked to our issue.

* @param { String[] } projection a list of Baladyia object attributes to keep
* @returns { Object | null } Returns a baldiya's object, or null
*/
(mattricule, code, bcode, projection)=>{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please fix linting issues

Copy link
Member Author

Choose a reason for hiding this comment

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

I liked everything and learned a lot from it 😁; there is no cause to dislike you, and thank you very much for your valuable feedbacks.

Comment on lines +12 to +14
* @param { integer } mattricule wilaya mattricule
* @param { integer } code daira code
* @param { integer } bcode baladiya code
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mean?

Suggested change
* @param { integer } mattricule wilaya mattricule
* @param { integer } code daira code
* @param { integer } bcode baladiya code
* @param { number } mattricule wilaya mattricule
* @param { number } code daira code
* @param { number } bcode baladiya code

@@ -0,0 +1,31 @@
const projectBaladiya = require('../utils/projections/projectObject');

const getBaladiyaByCodeAndDairaCodeAndWilayaCode = (data) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please do you have any use case for such a feature? I believe that Baladyia code is unique overall, so if you'd use it it won't matter to add extra params. And if you want to get a baladyia by wilaya or daira code you'd just use their methods with a projection

Copy link
Member Author

@abderrahmaneMustapha abderrahmaneMustapha Jun 17, 2021

Choose a reason for hiding this comment

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

can you please explain how can i get the same result by using projection

Copy link
Collaborator

@Fcmam5 Fcmam5 Jun 17, 2021

Choose a reason for hiding this comment

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

We do have these methods in place:

const {
  getBaladyiatsForWilaya,
  getBaladyiatsForDaira
} = require("@dzcode-io/leblad");

const rs1 = getBaladyiatsForWilaya(31); // if this wasn't implemented, we'd use getWilayaByCode and project only "dairats"

const rs2 = getBaladyiatsForDaira("ORAN");

Maybe it'd be nice to have another getBaladyiatsForDairaCode method. If you want to create it, please do it on a separate PR, based on develop

@@ -0,0 +1,38 @@
const projectBaladiya = require('../utils/projections/projectObject');

const getBaldiyaByCodeForWilaya = (data) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd name it getBaladyiaByWilayaCode. But again, why we don't just use getWilayaByCode then call the projection.

Comment on lines +12 to +13
* @param { Number } mattricule wilaya code (mattricule)
* @param { Number } code baladiya code (code)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please check jsdoc here and in all changed files

Suggested change
* @param { Number } mattricule wilaya code (mattricule)
* @param { Number } code baladiya code (code)
* @param { number } mattricule wilaya code (mattricule)
* @param { number } code baladiya code (code)

@@ -0,0 +1,31 @@
const projectBaladiya = require('../utils/projections/projectObject');

const getBaldiyatsForDairaByCodeForWilayaByCode = (data) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as my previous comments, plus this name can be simplified to getBaldiyatsByWilayaCode or getBaldiyatsByDairaCode. And if you could find a counterexample/argument to keep such composed methods which means that daira code is not enough to identify wilayas. we can name this: getBaldiyatsByWilayaAndDairaCodes

const getDairaByCodeAndWilayaCode = require('./api/getDairaByCodeAndWilayaCode');
const getBaladiyaByCodeAndDairaCodeAndWilayaCode = require('./api/getBaladiyaByCodeAndDairaCodeAndWilayaCode');
const getBaldiyatsForDairaByCodeForWilayaByCode = require('./api/getBaldiyatsForDairaByCodeForWilayaByCode');
const getBaldiyaByCodeForWilaya = require('./api/getBaldiyaByCodeForWilaya');
const data = require('../data/WilayaList.json');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd leave a blank line before the dataset import just for the sake of clarity and readablity

Comment on lines +36 to +39



it('should export a function', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please apply the linter+prettier to remove such whitespaces and to add an empty line at EOF

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Addressing Feedback
Development

Successfully merging this pull request may close these issues.

4 participants