Skip to content

Config option to disable GeoIP database - #2720

Closed
3meraldK wants to merge 4 commits into
AuthMe:masterfrom
3meraldK:master
Closed

3meraldK wants to merge 4 commits into
AuthMe:masterfrom
3meraldK:master

Conversation

@3meraldK

Copy link
Copy Markdown
Contributor

No description provided.

@ljacqu

ljacqu commented Jun 27, 2023

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor Author

Requesting @ljacqu to review

@ljacqu

ljacqu commented Sep 24, 2023

Copy link
Copy Markdown
Member

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
@ljacqu

ljacqu commented Sep 24, 2023

Copy link
Copy Markdown
Member

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants