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

feat: codec options argument for engine initialization #325

Closed
wants to merge 1 commit into from
Closed

feat: codec options argument for engine initialization #325

wants to merge 1 commit into from

Conversation

ivan-gj
Copy link

@ivan-gj ivan-gj commented Jan 29, 2023

The original idea was to resolve #118, which wants an easy feature to have all datetime.datetime fields as UTC-aware values.

This is definitely something very, very useful, as many people (including me) consider naive datetimes a source of problems.

The proposed solution in #118 is more field-specific, but considering the issue title and the fact that most people either want timezone awareness or not (everywhere), I feel like this is the best way of getting there.

The solution here tries to:

  • be simple to apply: generally, if someone is interested in timezone awareness, they will want this for all cases, so no need to configure it one by one (or maybe by document class).
  • not break current API: all projects already working should still work normally.
  • allow certain flexibility: through the CodecOptions object, we are able to not only define that timezone-aware datetimes is what we want, but also other conversion stuff that some people may find useful.

I must say that I believe that the solution could be cleaner if the AIOEngine and SyncEngine classes were not obfuscating the self.database object generation. I was thinking of allowing an input of a full database object, but technically it stops making sense to input a client, so I had to drop the idea, as having both client and database as input arguments stops making sense (the database contains a reference to the client). It would be dirtier, and a possible misuse could happen.

So if there are plans for breaking the API at a certain point, it should be considered to change the initialization of these engines, extracting the database object generation out of the BaseEngine subclasses, to facilitate custom configurations.

Close #118

@ivan-gj
Copy link
Author

ivan-gj commented Jan 29, 2023

Woops!
I just found that there are some pull requests #70 and #71 that basically take it a step further.

Sorry about this.

@ivan-gj ivan-gj closed this Jan 29, 2023
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.

Option to store all datetime as UTC instead of naive datetimes
1 participant