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

Which parsing bugs can we fix in a minor release? #1118

Open
pitdicker opened this issue Jun 1, 2023 · 4 comments
Open

Which parsing bugs can we fix in a minor release? #1118

pitdicker opened this issue Jun 1, 2023 · 4 comments

Comments

@pitdicker
Copy link
Collaborator

pitdicker commented Jun 1, 2023

To list a couple of issues:

I consider most of these real bugs. Parsing doesn't work as documented and doesn't match formatting (so no guaranteed round-tripping).

In my opinion the issue with #1112, which let to a revert of a backport of #807, is that we became stricter about whitespace parsing without fixing the parsing of padding.


All changes here can be observable. It is pretty much guaranteed there are format strings in use that happened to work, but would fail when we start fixing these issues.
But I don't think it is better to batch up all these changes, and wait until a major release. (And not make new changes after that.)

@pitdicker
Copy link
Collaborator Author

An other change that I can imagine would be better as an opt-in, or something to wait with until a major release, is #1094:

  • insert default values for some missing fields when parsing
  • error on unused fields

If there are incompatibilities, it would not require users to alter the format string but to alter the types that receive the parsed value.

@djc
Copy link
Contributor

djc commented Jun 1, 2023

There's a basic difference between "widening" changes that basically allow more format strings/input and "narrowing" changes that will generate errors of some kind for code that previously did something (hopefully useful). My usual approach would be that we should mostly stick to widening (allow minus sign, parse Z as offset).

For narrowing, I think there need to be very clear considerations in each PR about what the PR is breaking, an assessment how common that sort of usage might be (maybe backed up with data, by using GitHub code search or something?), and what the benefit is.

IMO it also makes a difference whether some change means we'll introduce new panic calls vs yielding an Err or None in some API that was already fallible.

But I don't think it is better to batch up all these changes, and wait until a major release. (And not make new changes after that.)

You don't think that is better? Consider that most downstream users will spend basically no effort verifying semver-compatible updates (other than running whatever tests they might have), whereas a semver-incompatible update requires that downstream users opt-in (as part of which they have a bigger chance of consuming release notes etc).

@pitdicker
Copy link
Collaborator Author

You write good replies in very little time...

For narrowing, I think there need to be very clear considerations in each PR about what the PR is breaking, an assessment how common that sort of usage might be (maybe backed up with data, by using GitHub code search or something?), and what the benefit is.

That seems like a good standard to me.

You don't think that is better? Consider that most downstream users will spend basically no effort verifying semver-compatible updates

Well, my thoughts are mixed about this is better to say. I agree with that. Yet making many changes to parsing without getting real-world testing for quite a while is also difficult?

@djc
Copy link
Contributor

djc commented Jun 1, 2023

Well, my thoughts are mixed about this is better to say. I agree with that. Yet making many changes to parsing without getting real-world testing for quite a while is also difficult?

Yup, but we can respond quickly with bugfix releases to mitigate the impact.

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

No branches or pull requests

2 participants