Skip to content

Increase backwards compatibility of moving raw bundles to Base modules #1865

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

Closed
MatthewDaggitt opened this issue Oct 25, 2022 · 12 comments · Fixed by #1867
Closed

Increase backwards compatibility of moving raw bundles to Base modules #1865

MatthewDaggitt opened this issue Oct 25, 2022 · 12 comments · Fixed by #1867
Milestone

Comments

@MatthewDaggitt
Copy link
Contributor

In commits 578e7a6 and 88942c3 we move the raw bundles from Properties to Base files.

We should publicly re-export the raw bundles in the Properties files from the Base files for backwards compatibility (putting them in the deprecation section).

@MatthewDaggitt MatthewDaggitt added this to the v2.0 milestone Oct 25, 2022
@jamesmckinna
Copy link
Contributor

I'll attempt to do this, but it sounds very like a 'deprecation-introduced-with-the-sole-purpose-of-being-subsequently-removed'... cf. my attempted gymnastics in trying to resolve issue #1726

... we've discussed backwards compatibility vs. breaking changes for the v2.0 milestone before, and my instincts are for the cleaner break.

But I'll see how it looks in practice.

@jamesmckinna
Copy link
Contributor

And yes, I do appreciate that deprecation is designed to help client code migrate towards the Promised Land... ;-)

@MatthewDaggitt
Copy link
Contributor Author

but it sounds very like a 'deprecation-introduced-with-the-sole-purpose-of-being-subsequently-removed'...

Yes, that is it exactly 😄

... we've discussed backwards compatibility vs. breaking changes for the v2.0 milestone before, and my instincts are for the cleaner break.

Yup I remember, but I still think where its easy for us to support backwards compatibility we should. This is only a couple of lines per file, no new imports etc.

@jamesmckinna
Copy link
Contributor

Does this require an internal Bundles module declaration in Nat.Base, so that the public reexport only covers the bundles?

@MatthewDaggitt
Copy link
Contributor Author

No. It should be as simple as adding:

open Data.Nat.Base public 
  using (+-rawMagma, ...)

to the bottom of the Data.Nat.Properties file.

@jamesmckinna
Copy link
Contributor

But doesn't that make the deprecation non-local? Why not do 'the usual' of defining the original names in the deprecated section equal to their new counterparts? That way the deprecation is in the imported Properties module, not the Base module... ?

@MatthewDaggitt
Copy link
Contributor Author

It does, but the problem is that when you're moving things, but not changing their name, then if you give the old location the same name then you're getting to get name clashes when you have both Base and Properties open....

jamesmckinna added a commit to jamesmckinna/agda-stdlib that referenced this issue Oct 25, 2022
jamesmckinna added a commit to jamesmckinna/agda-stdlib that referenced this issue Oct 25, 2022
jamesmckinna added a commit to jamesmckinna/agda-stdlib that referenced this issue Oct 26, 2022
@jamesmckinna
Copy link
Contributor

jamesmckinna commented Oct 26, 2022

OK, see PR #1867 , modulo the renaming of the exported properties in Data.Rational.*.Base.
Where should the deprecation happen: in Base, with the re-export only being of the deprecated names in Properties?
I think that's the next step...
...Or can this be done by deprecating names in Base, and in the re-export in Properties rename the new names to the old (and now deprecated ;-)) ones?
EDITED: still need to turn off the warnings, as with #1726 / #1868 ... sigh
UPDATED: done now?

@JacquesCarette
Copy link
Contributor

As I said on one of the PRs, I think this is a mistake. I think we should create Data.X.Bundles for these and NOT pollute poor .Base modules with Algebra-related matters. They can still be re-exported by .Properties, I expect those modules to be 'fat'.

@jamesmckinna
Copy link
Contributor

jamesmckinna commented Oct 28, 2022

Yes @JacquesCarette there seemed to be a clear disagreement/divergence of views between you on the one hand, and @Taneb and @MatthewDaggitt on the other in the original discussion of #1755 and subsequent issues/PRs.

I'm agnostic on this one (and currently trying to reverse out a separate hell of careless management of my PR closing this current issue), but I might be tempted to re-open the discussion in the context of #1871: if I were to persuade people (@MatthewDaggitt has yet to be convinced :-() that homomorphisms between structures X and Y should live outside Data.X.Properties (the present defualt setting), then that would go hand-in-hand with arguments to move the associated raw bundles out of Data.X|Y.Base...
... having said that, I perhaps do not expect Properties to (have to) re-export raw bundles: Ithtink that's the job of Data.X, surely?

But we're still stuck on the 'minimalist/maximalist' spectrum of how to view/build/advance stdlib for both developers and users alike: you're perhaps (?) an unusual hybrid, who also likes mimimal interfaces; others have speculated that 'users' (may/might) have a visibility/navigability problem, so tend towards the 'all-in-one-place' maximalist position.

(EDITED: apologies for the spurious mention, now remedied)

@MatthewDaggitt
Copy link
Contributor Author

As I said on one of the PRs, I think this is a mistake. I think we should create Data.X.Bundles for these and NOT pollute poor .Base modules with Algebra-related matters. They can still be re-exported by .Properties, I expect those modules to be 'fat'.

The Algebra.Bundles.Raw dependencies are minuscule compared to Algebra.Structures or Algebra.Bundlesbut I understand your reluctance.

I'm happy to create Data.X.Bundles and have them re-exported by Data.X (and Data.X.Properties for backwards compatibility).

if I were to persuade people (@MatthewDaggitt has yet to be convinced :-() that homomorphisms between structures X and Y should live outside Data.X.Properties (the present defualt setting),

It's not that I'm not convinced that its a good idea, it's just that I haven't seen a proposal that convinces me that it works yet 😉

@jamesmckinna
Copy link
Contributor

Touché ;-)!

jamesmckinna added a commit to jamesmckinna/agda-stdlib that referenced this issue Oct 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants