Skip to content

stringify fact keys by default - #189

Open
bastelfreak wants to merge 1 commit into
voxpupuli:masterfrom
bastelfreak:symbooooools
Open

stringify fact keys by default#189
bastelfreak wants to merge 1 commit into
voxpupuli:masterfrom
bastelfreak:symbooooools

Conversation

@bastelfreak

Copy link
Copy Markdown
Member

for a long long time, the first level of keys in a factsets were symbols. Following nested hashes always had strings. This doesn't make sense and looks confusing. Also the original data contains only strings, we we actually iterated on the data and converted it to symbols. We now flip the default. This saves performance and makes code more readable.

@bastelfreak bastelfreak self-assigned this Jun 21, 2024
@bastelfreak bastelfreak mentioned this pull request Jun 21, 2024
@bastelfreak
bastelfreak requested a review from ekohl June 21, 2024 13:36
@ekohl

ekohl commented Jun 24, 2024

Copy link
Copy Markdown
Member

I'd prefer to start with converting some modules to use symbols to see how it goes.

@bastelfreak

Copy link
Copy Markdown
Member Author

@ekohl I've one example here: voxpupuli/puppet-autofs#217
We need a major release of rspec-puppet-facts to get the latest FacterDB version. And I think it makes sense to switch to stringified facts already. That will save us one major release.

@bastelfreak

Copy link
Copy Markdown
Member Author

Another one: voxpupuli/puppet-ca_cert#113

@bastelfreak

Copy link
Copy Markdown
Member Author

Another one: voxpupuli/puppet-archive#527

@ekohl

ekohl commented Jun 27, 2024

Copy link
Copy Markdown
Member

I've summarized my thoughts in #188 (review)

@bastelfreak bastelfreak added this to the 6.0.0 milestone Jun 28, 2024
for a long long time, the first level of keys in a factsets were
symbols. Following nested hashes always had strings. This doesn't make
sense and looks confusing. Also the original data contains only strings,
we we actually iterated on the data and converted it to symbols. We now
flip the default. This saves performance and makes code more readable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants