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 in constructors with arguments, setters, updated Query class to reflect this and added in a test case #11

Merged
merged 5 commits into from
Feb 3, 2022

Conversation

byt3sage
Copy link

@byt3sage byt3sage commented Feb 2, 2022

Relating to: #10 #7

I had to do this quickly as I'm currently using this package inside a project so needed to get it working ASAP. I've added in class constructors for all the query classes as well as initialising these arguments within the constructor. I have also added in the GeoShape query to this with relating test. Furthermore, all Query classes now have setters within them.

Any feedback is welcome.

Sidenote: We need some more tests for other query classes. I'll do that once this is ready and merged.

CC @erichard

@byt3sage byt3sage mentioned this pull request Feb 2, 2022
@erichard
Copy link
Owner

erichard commented Feb 2, 2022

constructors arguments cannot be nullable if they are needed to build the query

@byt3sage
Copy link
Author

byt3sage commented Feb 2, 2022

You're correct - I misread what you actually wanted, I thought that was a tad strange.

Just to confirm @erichard , you want all needed attributes for a query to be set via a constructor and optional attributes to be set via setters?

@erichard
Copy link
Owner

erichard commented Feb 2, 2022

@jaetooledev Yes exactly. Sorry for not being clear 🙏

@byt3sage
Copy link
Author

byt3sage commented Feb 3, 2022

@erichard how about this? required arguments are set via constructors, optional arguments are set via setters.

If this is okay - might it be worth me removing the checks that non-optional fields are empty in some of the queries since with type hinting they can't be null

@erichard
Copy link
Owner

erichard commented Feb 3, 2022

Thanks. Looks good to me

@erichard erichard merged commit 9b7398b into erichard:main Feb 3, 2022
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