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

Atom Support #47

Merged
merged 19 commits into from
Feb 16, 2024
Merged

Atom Support #47

merged 19 commits into from
Feb 16, 2024

Conversation

ddkasa
Copy link
Contributor

@ddkasa ddkasa commented Feb 14, 2024

Made the changes as discussed in #46.

I made some directory changes to accommodate the new Atom model. Otherwise kept to the same code style as used for the RSS model. Works when testing with the sample Atom XML provided, but can't seem to get the tests working as the JSON dump formatting is giving me problems.

Let me know what you need to change before merging or how to approach the testing.

@dhvcc dhvcc assigned dhvcc and unassigned dhvcc Feb 14, 2024
@dhvcc dhvcc self-requested a review February 14, 2024 20:36
@ddkasa ddkasa marked this pull request as ready for review February 15, 2024 11:59
Copy link
Owner

@dhvcc dhvcc left a comment

Choose a reason for hiding this comment

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

I'll go over standarts for Atom a bit later, here's a couple of comments regarding the structure

tests/test_parsing.py Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
rss_parser/_parser.py Show resolved Hide resolved
@dhvcc
Copy link
Owner

dhvcc commented Feb 15, 2024

Nice PR, as I said, I'll go over spec for Atom a bit later, however, thanks for contributing, if any of the parts from my comments are unclear or you don't have time to implement them - ping me, I'll help answering or coding

@dhvcc
Copy link
Owner

dhvcc commented Feb 15, 2024

Also, this should fix tests

--- a/tests/samples/atom.json
+++ b/tests/samples/atom.json
@@ -63,7 +63,7 @@
             },
             "published": {
               "attributes": {},
-              "content": "2003-12-13 08:29:29-04:00"
+              "content": "2003-12-13T08:29:29-04:00"
             },
             "rights": null,
             "source": null,
@@ -126,7 +126,7 @@
       },
       "updated": {
         "attributes": {},
-        "content": "2005-07-31 12:29:29+00:00"
+        "content": "2005-07-31T12:29:29Z"
       }
     }
   },

@ddkasa
Copy link
Contributor Author

ddkasa commented Feb 15, 2024

Alright, tests are working now as well. Let me know what I need to change for the Atom spec as well.

Copy link
Owner

@dhvcc dhvcc left a comment

Choose a reason for hiding this comment

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

Hey, know this is a lot of comments, but it's just due to us not having a clear plan of development before implementation. I'll try to raise a PR for your branch in your repo so we can merge it faster

rss_parser/_parser.py Show resolved Hide resolved
rss_parser/models/atom/feed.py Outdated Show resolved Hide resolved
rss_parser/models/atom/feed.py Outdated Show resolved Hide resolved
rss_parser/models/atom/feed.py Outdated Show resolved Hide resolved
rss_parser/models/atom/feed.py Outdated Show resolved Hide resolved
rss_parser/models/atom/entry.py Outdated Show resolved Hide resolved
rss_parser/models/atom/entry.py Outdated Show resolved Hide resolved
rss_parser/models/atom/entry.py Outdated Show resolved Hide resolved
rss_parser/models/atom/entry.py Show resolved Hide resolved
rss_parser/models/atom/entry.py Show resolved Hide resolved
@dhvcc
Copy link
Owner

dhvcc commented Feb 15, 2024

@ddkasa Whoops, accidentally commited straight to your branch, I apologize, wanted to create a branch to show you the diff with the fixes. Please check out the diff and let me know what you think

There can be a case where system timezone setting may break
datetime.fromtimestamp if supplying a timezone like `...Z`
Pydantic module already has it, so re-using it (as it should've been before)
Copy link
Owner

@dhvcc dhvcc left a comment

Choose a reason for hiding this comment

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

LMK if this seems good to you and I'll merge

@ddkasa
Copy link
Contributor Author

ddkasa commented Feb 16, 2024

Looks good to me.

Initially had a bunch of sub-models setup, but decided to remove before submitting to keep it more simple.

Regarding removing the root_key. There might be some deprecation issues with that and also in terms of syntax further down the line would be awkward as you would have to use parsed_model.rss.foo... before accessing the feed every time.

@dhvcc
Copy link
Owner

dhvcc commented Feb 16, 2024

@ddkasa there is a way to backport even when removing, but I agree on using model.rss.blah.blah
I'll think about it, but I think it's ok. I'll prepare a release after the decision, let me know if this is urgent for you/your project

@dhvcc dhvcc merged commit 1143adb into dhvcc:master Feb 16, 2024
12 checks passed
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