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 NormalizeFieldKeys middleware #473

Merged
merged 15 commits into from
Feb 29, 2024

Conversation

Technologicat
Copy link
Contributor

@Technologicat Technologicat commented Feb 19, 2024

Here's an attempt at #467, which see.

Fixes #467

@tdegeus
Copy link
Collaborator

tdegeus commented Feb 19, 2024

(Thanks BTW ;))

@Technologicat
Copy link
Contributor Author

(Thanks BTW ;))

(np BTW ;))

@Technologicat
Copy link
Contributor Author

Also, noticed that black was complaining, so reformatted both fieldkeys.py and test_fieldkeys.py using it.

@tdegeus
Copy link
Collaborator

tdegeus commented Feb 23, 2024

Thanks @Technologicat !! Since I had a few minutes to spare, I offered some simplifying suggestions by adding to commits. Please don't hesitate to criticize / complement / revert.

@MiWeiss MiWeiss changed the title add NormalizeFieldKeys middleware ✨ Add NormalizeFieldKeys middleware Feb 26, 2024
Copy link
Collaborator

@tdegeus tdegeus left a comment

Choose a reason for hiding this comment

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

Good to go from my side!

@Technologicat
Copy link
Contributor Author

4b328ba: Ok, nice shortened comment. Thanks.
6ad724f: Ok.
9eed680: Ok.

71ee1de: Nice shortening of the test unit. Thanks!

Regarding this commit, some minor comments:

Contrary to the docstring, the function test_normalize_fieldkeys is actually testing with lowercase keys. I think the docstring should be fixed. This is important, because as the old maxim goes, "if code and comments disagree, both are probably wrong".

Personally, I'd also avoid the name "foo{i}". A semantically meaningful name, such as "entry{i}", would increase clarity by making the intent explicit. Similarly, in test_normalize_fieldkeys_force_last, the bare "foo" could be e.g. "dummyvalue" to make it clear at a glance what it stands for.

(Using meaningful names exclusively is a habit I picked up from Racket docs ~half a decade ago. The standard metasyntactic names were fine in the 1970s, but we know better now. :) )

@tdegeus
Copy link
Collaborator

tdegeus commented Feb 28, 2024

Sounds good! Would you like to make the changes?

@Technologicat
Copy link
Contributor Author

Sure, here they are. :)

@Technologicat
Copy link
Contributor Author

Ugh, a merge conflict. Fixed. And some flake8 warnings fixed, too.

Should be fine by me now.

@tdegeus
Copy link
Collaborator

tdegeus commented Feb 29, 2024

Thanks a million!

@tdegeus tdegeus merged commit 09f8bc1 into sciunto-org:main Feb 29, 2024
18 checks passed
@Technologicat
Copy link
Contributor Author

Thanks for the merge!

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.

Normalize field keys (to lowercase)
3 participants