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.

Sign up for GitHub

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

Jump to bottom

Config option to disable GeoIP database #2720

Closed
wants to merge 4 commits into from
Closed

Config option to disable GeoIP database #2720

wants to merge 4 commits into from
+21 −0

Conversation

3meraldK
Copy link
Contributor

No description provided.

@3meraldK
Config option to disable GeoIP database

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
803ea5c
@ljacqu
Copy link
Member

ljacqu commented Jun 27, 2023

I don't think the constructor is the correct place to check for this, but rather in isDataAvailable(). It's not just called by the constructor but also in lookup methods, so it would be better to properly catch it in isDataAvailable and to avoid conditional logic in the constructor.

@sgdc3 sgdc3 requested review from ljacqu and sgdc3 July 16, 2023 17:25
@3meraldK
Copy link
Contributor Author

Requesting @ljacqu to review

@ljacqu
Copy link
Member

ljacqu commented Sep 24, 2023

Two unit tests fail, and I think we don't need the enabled field since all public data methods go through getCountry() -> I will fix this

@ljacqu ljacqu self-assigned this Sep 24, 2023
ljacqu pushed a commit that referenced this pull request Sep 24, 2023
@3meraldK @ljacqu
Config option to disable GeoIP database #2720
65a272d
@ljacqu ljacqu mentioned this pull request Sep 24, 2023
Merged
@ljacqu
Copy link
Member

ljacqu commented Sep 24, 2023

The changes are now on master. Thanks!

@ljacqu ljacqu closed this Sep 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgdc3 sgdc3

@ljacqu ljacqu

Assignees

@ljacqu ljacqu

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@3meraldK @ljacqu @sgdc3