Skip to content

Conversation

@macobo
Copy link
Contributor

@macobo macobo commented Jul 9, 2024

In the main repo we'll be starting to sync locations into ClickHouse. For that to work, we need two new features:

  • Getting all locations out of the library (via &all/0 methods)
  • Having functionality for knowing the version of the library

Related PR: plausible/analytics#4328

Copy link
Member

@aerosol aerosol left a comment

Choose a reason for hiding this comment

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

Change requests inline.

nitpick: modules and functions instead of classes and methods 🙈 - I'm sorry, I'll show myself out

@macobo macobo changed the title Support &all/0 in all classes, Location.version() method Support &all/0 in all modules, Location.version() function Jul 10, 2024
@macobo macobo requested a review from ruslandoga July 10, 2024 08:00
def version() do
version_file = Application.app_dir(:location, "priv/version")

File.read!(version_file)
Copy link
Contributor

@ruslandoga ruslandoga Jul 10, 2024

Choose a reason for hiding this comment

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

Can it be compiled into the function? I'd like to remove most of the runtime behaviour from this library (wanted to discuss it with @ukutaht on the next call) ... Gettext-style, kind of. Probably doesn't matter since it wouldn't change the public API.

@ruslandoga
Copy link
Contributor

ruslandoga commented Jul 10, 2024

This PR would probably need to add tests for the new all/0 functions. They assume a certain ets table structure that might change in the future.

@macobo macobo requested a review from ruslandoga July 11, 2024 13:12
@macobo macobo merged commit a89bf79 into main Aug 12, 2024
@macobo macobo deleted the all-all branch August 12, 2024 06:39
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.

4 participants