Skip to content

Conversation

@tshrv
Copy link

@tshrv tshrv commented May 21, 2021

I've added two more filters:

  • Vaccine (covaxin/covishield)
  • Dose (first/second)

Updated filter function mechanism to accommodate multiple filters with a dry approach.
Updated param typo min_age_limt -> min_age_limit

@DragonWarrior15
Copy link
Contributor

If you could contrast this with PR #15 and PR #17, would be great.

@tshrv
Copy link
Author

tshrv commented May 22, 2021

Sure @DragonWarrior15,

The following are the features present in PR #25 in contrast with PR #15 and PR #17 -

  1. Instead of putting filters in a dictionary, putting them as the arguments will make it more editor friendly and will have less scope of human error.
  2. Filter Vaccine is an Enum. Editor friendly and reduce chances of human error. cowin_api.constants.py
  3. Filter Dose is an Enum, to filter centers for availability(atleast 1) of the first or second dose. cowin_api.constants.py
  4. Since there are limited number of parameters based on which we might need to filter (currently min_age_limit, vaccine, dose), it is better to have a simpler filter mechanism even if it involves explicitly checking for multiple filters. cowin_api.utils.py

Missing features:

  1. Filter Fees

@DragonWarrior15
Copy link
Contributor

DragonWarrior15 commented May 22, 2021

Instead of putting filters in a dictionary, putting them as the arguments will make it more editor friendly and will have less scope of human error.

Not sure how this works. Dictionary is also a well formatted structure in any editor.

Filter Vaccine is an Enum. Editor friendly and reduce chances of human error. cowin_api.constants.py
Isn't this tedious than simply passing a string as input ? I understand that you are trying to avoid spelling mistakes here.

Further, you maybe limited to what vaccines are put in the Enum, and the API will keep evolving as more and more vaccines are being brought in.

Since there are limited number of parameters based on which we might need to filter (currently min_age_limit, vaccine, dose), it is better to have a simpler filter mechanism even if it involves explicitly checking for multiple filters. cowin_api.utils.py

You never know how this will keep growing, so your explicit filtering will require multiple modifications to the code as it evolves. I mean both arguments are kind of right here I guess.

Filter Fees

This can be easily added in the dictionary. The core user functions that take arguments are simply there for compatibility with older versions of the API.

@tshrv
Copy link
Author

tshrv commented May 22, 2021

Sure, You're right. It's already great.

@tshrv tshrv closed this May 22, 2021
@DragonWarrior15
Copy link
Contributor

Hey, you can keep the PR open, this merged with PR #17 will probably be useful. Not sure why you closed it.

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