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

Quotes consistency #2326

Merged
merged 3 commits into from
Sep 25, 2023
Merged

Quotes consistency #2326

merged 3 commits into from
Sep 25, 2023

Conversation

zas
Copy link
Collaborator

@zas zas commented Sep 23, 2023

Summary

  • This is a…
    • Bug fix
    • Feature addition
    • Refactoring
    • Minor / simple change (like a typo)
    • Other
  • Describe this change in 1-2 sentences:

Problem

  • JIRA ticket (optional): PICARD-XXX

Solution

Action

@zas zas requested review from rdswift and phw September 23, 2023 21:05
@zas
Copy link
Collaborator Author

zas commented Sep 23, 2023

Not finished, but the idea is to go through all files.
Overall, it appears we were mainly following quoting guidelines (well, in fact guidelines were made after our practice), but there are still few inconsistencies.

I clarified quoting of URIs: basically a quote can theoretically appear in those (single quote is a reserved character for sub delimitation, a bit like # or ?). So better double quote those.

Copy link
Collaborator

@rdswift rdswift left a comment

Choose a reason for hiding this comment

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

These look good. The convention also makes sense. Easy to understand, and it makes the code easier to read.

@zas zas force-pushed the quotes_consistency branch from 2116dd7 to 6cdd8f9 Compare September 24, 2023 08:38
@zas zas marked this pull request as ready for review September 24, 2023 10:32
@zas zas requested a review from rdswift September 24, 2023 10:34
Copy link
Collaborator

@rdswift rdswift left a comment

Choose a reason for hiding this comment

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

This all looks good to me. Now I just need to try to remember to follow it when submitting changes. ;-)

zas added 3 commits September 25, 2023 07:41
- Add a note about URI & quoting
- Add note about XML
- Clarify scope and usage if in doubt

Clarification
In fact, Python codec is `utf_8` but it has aliases, `utf8' (declared as
alias) or `utf-8` (because `_` or `-` do not make difference) are
working, but we mainly use `utf-8` among the code.

See https://docs.python.org/3/library/codecs.html#standard-encodings
@phw phw force-pushed the quotes_consistency branch from 0a630c5 to 511f3be Compare September 25, 2023 05:43
@phw phw merged commit 15bf226 into metabrainz:master Sep 25, 2023
51 checks passed
@phw
Copy link
Member

phw commented Sep 25, 2023

Merged, but I squashed all the commits that updated the quotes only

@zas
Copy link
Collaborator Author

zas commented Sep 25, 2023

Merged, but I squashed all the commits that updated the quotes only

In general, I wouldn't squash such big set of changes, it disables the possibility to bisect. I intentionally split changes in small sets.
In this case, that's mainly stylistic changes, and they shouldn't have impact on code execution though.

@phw
Copy link
Member

phw commented Sep 25, 2023

In this case it was really a huge amount of separate commits with essentially all the same commit message and with many but trivial changes that causes parser error if done wrong. There was not much point in keeping those.

I totally agree in doing changes in multiple commits and I'm not much a friend of squashing all PR as some projects practice it, as any PR that is a bit more complex easily consists of multiple separate steps of implementation. But if each commit is just an extension or fix of the previous one we can keep them combined.

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.

3 participants