Skip to content

Return a POSIXct timestamp instead of a non-standard locale dependent date#416

Merged
jmaspons merged 2 commits intomainfrom
POSIXct
Mar 17, 2026
Merged

Return a POSIXct timestamp instead of a non-standard locale dependent date#416
jmaspons merged 2 commits intomainfrom
POSIXct

Conversation

@jmaspons
Copy link
Collaborator

No description provided.

@jmaspons jmaspons requested a review from mpadge March 16, 2026 18:38
@mpadge
Copy link
Member

mpadge commented Mar 17, 2026

@jmaspons Thanks for picking this one up too. There was a reason for that originally, which I unfortunately forget the exact details of. It was related to what happens when writing and re-reading an XML doc via osmdata_xml(). Can you maybe just add a test to demonstrate that your updates here are successfully carried across with a workflow like this:

f <- "doc.osm"
osmdata_xml(q, filename = f)
x <- osmdata_sf(q, doc = f)


That makes me also realise that we've got an inconsistency there, with filename in osmdata_xml, and doc in the read functions (_sf/_sc). Looking at the git blame, it seemed that was introduced in 920fae2. We should make the nomenclature consistent there, which would be a potentially breaking change that would require a deprecation step.

Copy link
Member

@mpadge mpadge 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 request changes here, just so I see your response to #416 (comment). Thanks!

@jmaspons
Copy link
Collaborator Author

I added tests to compare timestamps for results from queries and from doc.

For the filename / doc, it has been always filename for osmdata_xml() (parameter introduced in fdf15f0), but we can change to doc and deprecate filename. I'm not convinced by the doc naming better xml or data, but it works and there is no need to deprecate all the osmdata_*(). I'will do it in another PR.

@jmaspons jmaspons requested a review from mpadge March 17, 2026 09:01
@mpadge
Copy link
Member

mpadge commented Mar 17, 2026

@jmaspons I don't mind at all whether they're called doc, filename, or even something else, but just thought it would be good to be consistent. Then again, filename names an output file, whereas doc names an input file. That might be an argument for having different names and ignoring what I said anyway. It was just a thought; happy to leave up to you. thanks 👍

Copy link
Member

@mpadge mpadge left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@jmaspons jmaspons merged commit 1bfdb0e into main Mar 17, 2026
9 checks passed
@jmaspons jmaspons deleted the POSIXct branch March 17, 2026 20:03
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