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

Add repository timestamps #169

Closed
wants to merge 7 commits into from
Closed

Conversation

dsnipe
Copy link
Contributor

@dsnipe dsnipe commented Apr 1, 2015

PR based on discussion in #134

#
# class User
# include Lotus::Entity
# attributes :timestamps
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about having a separate method, for example add_timestamps instead? IMHO I think we should allow users to have attribute with naming 'timestamps'.

Copy link
Member

Choose a reason for hiding this comment

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

Why do we have 2 different ways in defining timestamp? For repo, we do include Lotus::Repository::Timestamps why don't we do the same here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joneslee85 separate method is good solution, you're right.
But what do you think about dynamical creation of #created_at and #updated_at? After all, 3 places for declaration timestamps is a little uncomfortable.
Users can declare it in repository and in maps (inside collection). After an entity will be persisted and returned, they'll be have timestamps.
Also I think about make them read-only for users. What do you think?

@dsnipe
Copy link
Contributor Author

dsnipe commented Apr 5, 2015

@joneslee85 thanks for your comments.
Actually, this feature not finished yet (draft). I decided to first implement a dirty checks, it helps with update_at and then move on.
I have some ideas of realisation now, but need some time to structure it. Today will do and post it for discussion.

@dsnipe
Copy link
Contributor Author

dsnipe commented Apr 6, 2015

btw, #170 will be very useful here for checking need update #updated_at or not. Thereby, I'll wait till it merged (or declined for some reason) and use it functionality.

@runlevel5
Copy link
Member

@dsnipe can you please rebase?

@dsnipe
Copy link
Contributor Author

dsnipe commented Apr 20, 2015

@joneslee85 done
btw, I finish it on this week, I think

@runlevel5
Copy link
Member

@dsnipe thanks for the update, looking forward to seeing your 2nd PR merge

@dsnipe
Copy link
Contributor Author

dsnipe commented Apr 21, 2015

@joneslee85, @jodosha
most work is done, but I have couple thing that should be discussed:

  1. In this version, developers should include timestamps modules and helpers in 3 places (repository, in entity and collections). For entities it can be included globally in future in prepare configuration block. What do you think about it? It's ok how it is, or maybe somehow create updated_at and created_at 'magically' in entity after developer included module in Repository and collection?
  2. As dirty tracking is in separate module, I can't manage state of entity, so updated_at always will be changed if entity going to persisted, even if it's not changed at all. I can came up with something if we need it (and I think in this case – how to not implement dirty tracking functionality again :) ). Which behavior is correct in this case?

let(:article) { Article.new(title: 'New article with timestamp') }
let(:persisted_article) { ArticleRepository.create(article) }

it '#created_at' do
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about using describe instead of it here? And describe what it does with it, for example: it 'touches created_at attribute'

@runlevel5
Copy link
Member

@dsnipe thanks for this work

In this version, developers should include timestamps modules and helpers in 3 places (repository, in entity and collections). For entities it can be included globally in future in prepare configuration block. What do you think about it? It's ok how it is, or maybe somehow create updated_at and created_at 'magically' in entity after developer included module in Repository and collection?

Atm, I have no strong opinion on using global configuration block. @jodosha please weight in your opinion on this

However, I don't think doing magic with entity through repository is a good idea. I want to keep Entity free from Repository knowledge and the other way around.

As dirty tracking is in separate module, I can't manage state of entity, so updated_at always will be changed if entity going to persisted, even if it's not changed at all. I can came up with something if we need it (and I think in this case – how to not implement dirty tracking functionality again :) ). Which behavior is correct in this case?

Are you implying about that upon calling persist or update, though there are no changes, the updated_at timestamp is still touched? If so, I am open to suggestion how to NOT implement like dirty tracking (as of Rails for eg).

@dsnipe
Copy link
Contributor Author

dsnipe commented Apr 22, 2015

@joneslee85 thanks for the review

Atm, I have no strong opinion on using global configuration block. @jodosha please weight in your opinion on this

@jodosha write about it here: #172 (comment)
I like that idea, because it gives opportunity to extend existed functionality of Entities just putting your own module in prepare block.

However, I don't think doing magic with entity through repository is a good idea. I want to keep Entity free from Repository knowledge and the other way around.

Same for me actually.

Are you implying about that upon calling persist or update, though there are no changes, the updated_at timestamp is still touched? If so, I am open to suggestion how to NOT implement like dirty tracking (as of Rails for eg).

Yep. It tap updated_at even if Entity is not changed. I'll think about suggestions and open to ideas to :)

@runlevel5
Copy link
Member

@dsnipe I think improvement on top this feature can be done as separate PR.

@dsnipe
Copy link
Contributor Author

dsnipe commented Apr 23, 2015

@joneslee85 agree with you, so leave this PR as it is.
change naming in tests
did my best, naming is difficult task for me sometimes :)

@AlfonsoUceda
Copy link
Contributor

👍

# timestamps
# end
# end

Copy link
Member

Choose a reason for hiding this comment

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

please remove this empty line

@dsnipe
Copy link
Contributor Author

dsnipe commented May 13, 2015

Finally, I freed some time to continue :)
@joneslee85 after rebase some tests failed.
I suspect that's because in my fixtures I include to Artcile Timestamps module – https://github.com/lotus/model/blob/master/test/fixtures.rb#L8
Not sure what's to do. Easy way is to replace an Article to User class. But maybe better solution is split fixtures or tests?

@jodosha
Copy link
Member

jodosha commented May 15, 2015

@dsnipe Thanks for this PR and I'm terribly sorry for the communication problem that we had here. My apologize 🙏
I'm closing this, as this feature was already implemented by #173 #176 #182.

/cc @joneslee85

@jodosha jodosha closed this May 15, 2015
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