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

Saved segments/create table #4797

Merged
merged 4 commits into from
Nov 11, 2024
Merged

Saved segments/create table #4797

merged 4 commits into from
Nov 11, 2024

Conversation

apata
Copy link
Contributor

@apata apata commented Nov 11, 2024

Changes

Creates the saved segments table.

         List of data types
 Schema |     Name     | Description 
--------+--------------+-------------
 public | segment_type | 

                                           Table "public.segments"
    Column    |              Type              | Collation | Nullable |               Default                
--------------+--------------------------------+-----------+----------+--------------------------------------
 id           | bigint                         |           | not null | nextval('segments_id_seq'::regclass)
 name         | character varying(255)         |           | not null | 
 type         | segment_type                   |           | not null | 'personal'::segment_type
 segment_data | jsonb                          |           | not null | 
 site_id      | bigint                         |           | not null | 
 owner_id     | bigint                         |           |          | 
 inserted_at  | timestamp(0) without time zone |           | not null | 
 updated_at   | timestamp(0) without time zone |           | not null | 
Indexes:
    "segments_pkey" PRIMARY KEY, btree (id)
    "segments_site_id_index" btree (site_id)
Foreign-key constraints:
    "segments_owner_id_fkey" FOREIGN KEY (owner_id) REFERENCES users(id)
    "segments_site_id_fkey" FOREIGN KEY (site_id) REFERENCES sites(id) ON DELETE CASCADE

Tests

  • This PR does not require tests

Changelog

  • This PR does not make a user-facing change

Documentation

  • This change does not need a documentation update

Dark mode

  • This PR does not change the UI

def change do
create table(:segments) do
add :name, :string, null: false
add :personal, :boolean, default: true, null: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Should this be an enum of sorts? Are we certain this will remain 2-option list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the first iteration, I think we'll have 2 cases. If we end up needing more cases, we can always migrate to a "type" column down the road.

Copy link
Contributor

Choose a reason for hiding this comment

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

These migrations are generally a pain and so is dealing with booleans in code. I suggest switching to an enum: https://hexdocs.pm/ecto/Ecto.Enum.html - even in the 2-option case this will IMO result in cleaner code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you give an example of a potential issue with dealing booleans in code? As for cleaner code, I'm not so sure about that. Atoms are convenient in the backend, but using enums in the FE can be quite verbose.

import SegmentType from '../../segments.ts' 
// ...
if (segment.type === SegmentType.personal) {
// ...
}

vs

if (segment.personal) {
// ...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the first option. Elixir sample as well:

handle(segment.personal, segment.segment_data)

def handle(true, segment_data), do: ...
def handle(false, segment_data), do: ...

vs

handle(segment.type, segment.segment_data)

def handle(:personal, segment_data), do: ...
def handle(:team, segment_data), do: ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, it's not something that I'm currently branching functions on (see here: https://github.com/plausible/analytics/tree/saved-segments-spec/). The branching is happening in Ecto queries.

As for the example you gave, I wonder whether a better spec would actually be not to split the segment to pieces in function arguments. That seems to have been a successful strategy with Plausible.Query.

  @spec handle(Plausible.Segment.t()) :: :ok

  def handle(%Plausible.Segment{personal: true} = segment) do end
  def handle(%Plausible.Segment{personal: false} = segment) do end

@apata apata added this pull request to the merge queue Nov 11, 2024
Merged via the queue into master with commit b22b357 Nov 11, 2024
11 checks passed
apata added a commit that referenced this pull request Nov 14, 2024
* Add migration for Saved Segments

* Remove premature optimisation

* Format

* Refactor to explicit segment type
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