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

🥔✨ Journal: extract Terms from Entrys #1633

Merged
merged 4 commits into from
Jul 13, 2023
Merged

🥔✨ Journal: extract Terms from Entrys #1633

merged 4 commits into from
Jul 13, 2023

Conversation

zspencer
Copy link
Member

@zspencer zspencer commented Jul 4, 2023

Not yet linking to them; but that should be a decent next step.

zspencer
zspencer commented Jul 4, 2023
View reviewed changes
app/furniture/journal/entry.rb Show resolved Hide resolved
app/furniture/journal/term.rb Outdated Show resolved Hide resolved
app/furniture/journal/entry.rb Outdated Show resolved Hide resolved
app/furniture/journal/renderer.rb Outdated
doc.gsub(/@([a-zA-Z\d]*)@(.*\.[a-zA-Z]*)/, '<a href="https://\2/@\1">@\1@\2</a>')
doc
.gsub(/@([a-zA-Z\d]*)@(.*\.[a-zA-Z]*)/, '<a href="https://\2/@\1">@\1@\2</a>')
# .gsub(/#(\w+)/, '<a href="../terms/\1">#\1</a>')
Copy link
Member Author

Choose a reason for hiding this comment

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

Leftovers; just wanted to commit it before I fell asleep.

Copy link
Member

Choose a reason for hiding this comment

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

Should this commented-out code be removed, or are you keeping it for the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should not be in this branch; but the follow on one...

@zspencer zspencer marked this pull request as draft July 4, 2023 07:42
@zspencer zspencer added ✨ feature Reduces Client's Burden or Grants them Benefits 🥔 Satisfices It's good enough to use, but not particularly great labels Jul 6, 2023
zspencer added a commit that referenced this pull request Jul 10, 2023
@zspencer
🧹Journal: Make relationships of Entry Rails-able
b7f65da 
- #1633

I noticed I'd left the active-record relationship cables on the
`Journal::Entry` loose. Getting rid of `delegate`s in favor of the
`has_one :x, through: :y`
@zspencer zspencer mentioned this pull request Jul 10, 2023
Merged
@zspencer zspencer force-pushed the journal/introduce-terms branch from eb103c2 to 9cfdb7b Compare July 10, 2023 02:39
@zspencer zspencer changed the base branch from main to journal/tidy-entry-relations July 10, 2023 03:18
Base automatically changed from journal/tidy-entry-relations to main July 13, 2023 00:09
zspencer added a commit that referenced this pull request Jul 13, 2023
@zspencer
🥗🧹Journal: Make relationships of Entry Rails-able (#1648)
7df4c96 
- #1633

I noticed I'd left the active-record relationship cables on the
`Journal::Entry` loose. Getting rid of `delegate`s in favor of the
`has_one :x, through: :y`
@zspencer zspencer force-pushed the journal/introduce-terms branch from 632529a to e3fd349 Compare July 13, 2023 00:11
@zspencer zspencer marked this pull request as ready for review July 13, 2023 00:36
@zspencer zspencer requested review from a team July 13, 2023 00:52
@anaulin
Copy link
Member

anaulin commented Jul 13, 2023

Are these meant to be... tags? keywords? something else? I feel like using more "standard" words from the blogging world might make it easier for both coders and people using the app.

anaulin
anaulin reviewed Jul 13, 2023
View reviewed changes
db/schema.rb Outdated
create_table "journal_terms", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t|
t.uuid "journal_id"
t.string "canonical_term"
t.text "aliases"
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to make this an array, or something more structured than just text (which I assume right now it means to be comma-separated?).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll give it a shot! I have not used array types at the database level before but it shouldn't be too hard.

@anaulin
Copy link
Member

anaulin commented Jul 13, 2023

Oh, I see now that you are extracting hashtags from the body. So maybe "tags" would be a better name?

anaulin
anaulin approved these changes Jul 13, 2023
View reviewed changes
Copy link
Member

@anaulin anaulin left a comment

Choose a reason for hiding this comment

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

Nothing blocking, but I would love for this new thing to be named in a more "standard" way, e.g. "tags" or "keyword".

@zspencer
Copy link
Member Author

I like Keyword! I just didn't want to use Tag or Hashtag

@zspencer zspencer force-pushed the journal/introduce-terms branch from d8ac9bd to 1f7e22e Compare July 13, 2023 22:33
@zspencer
Apply feedback from Ana's code review:
06d3df7 
- Rename to `Keyword`
- Use an `array` field!
- validate!
@zspencer zspencer force-pushed the journal/introduce-terms branch from 1f7e22e to 06d3df7 Compare July 13, 2023 22:34
@zspencer zspencer merged commit 7d965ad into main Jul 13, 2023
@zspencer zspencer deleted the journal/introduce-terms branch July 13, 2023 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anaulin anaulin anaulin approved these changes

Assignees
No one assigned
Labels
✨ feature Reduces Client's Burden or Grants them Benefits 🥔 Satisfices It's good enough to use, but not particularly great
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zspencer @anaulin