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

Move four modules to graveyard #10248

Merged
merged 5 commits into from
Jan 10, 2019
Merged

Move four modules to graveyard #10248

merged 5 commits into from
Jan 10, 2019

Conversation

narimiran
Copy link
Member

@narimiran narimiran commented Jan 9, 2019

Rationale:

These four modules (subexes, scgi, smtp, oids) are poorly documented, not used in the stdlib, there are no tests for them, and aren't much used in the wild.

They will be added to graveyard so they can be installed via nimble.


Please do NOT squash commits when merging, in case one of them needs to be reverted later.

@andreaferretti
Copy link
Collaborator

Just a reminder:

Fixes #2796
Fixes #9741
Fixes nim-lang/smtp#2

@narimiran
Copy link
Member Author

And with asyncio already in lib/deprecated, removing scgi also fixes #8298.

@dom96
Copy link
Contributor

dom96 commented Jan 10, 2019

This is pretty breaking. I can think of at least two of my projects that use oids and at least one that uses smtp, why remove it?

By the way, merging your own PR is really bad, especially for something like this...

@timotheecour
Copy link
Member

timotheecour commented Jan 10, 2019

/cc @dom96 workaround: this makes things work again:

nimble install oids
nimble install smtp

a few more points:

By the way, merging your own PR is really bad, especially for something like this...

indeed. A thumbs up means: I like the spirit of the PR, it doesn't mean "I approve this PR after a proper review".
That being said, merging your own PR if someone approved it should be fine. The definition of "someone" is subject to debate, see https://irclogs.nim-lang.org/09-01-2019.html#08:17:44 for the whole discussion thread around it /cc @Araq

@narimiran
Copy link
Member Author

narimiran commented Jan 11, 2019

merging your own PR is really bad

It was discussed internally previously and approved, I wouldn't merge it otherwise.

@narimiran
Copy link
Member Author

It was discussed internally previously and approved

...but, after some more discussion, SMTP and OIDS are back :) (0a2f711)

@andreaferretti
Copy link
Collaborator

I can think of at least two of my projects that use oids and at least one that uses smtp

@dom96 That's not a good reason to avoid moving modules into graveyard: your projects can simply add a nimble dependency.

I have no opinion on smtp, but it is very weird to have oid into the stdlib since:

  • it is something similar to a UUID (something common to many languages) but not compatible with its spec
  • instead, it is derived from MongoDB concept of OID
  • and still fails to ba compatible with the MongoDB spec

We should implement UUIDs and remove this module

@dom96
Copy link
Contributor

dom96 commented Jan 11, 2019

@dom96 That's not a good reason to avoid moving modules into graveyard: your projects can simply add a nimble dependency.

That's true. And yes, in general I'm happy for this to happen, but I want us to have a good discussion about it. What exactly belongs in a stdlib and what doesn't?

One thing I would like to stress though: how are users supposed to know that they can get these modules from graveyard? We need a transition period for these otherwise our users will just get frustrated. @narimiran Instead of removing these modules, can you replace the file's contents with {.error: "This module has moved to the graveyard, use it by adding `requires "subexes"` to your .nimble file"}?

@timotheecour
Copy link
Member

timotheecour commented Jan 11, 2019

What exactly belongs in a stdlib and what doesn't?

that's a big topic and deserves a separate dedicated discussion in a github issue for easy cross-reference

Instead of removing these modules, can you replace the file's contents with {.error: "This module has moved to the graveyard, use it by adding requires "subexes" to your .nimble file"}?

yes! EDIT: won't that break the workaround nimble install oids ? /cc @dom96

We need a transition period for these otherwise our users will just get frustrated

in general yeah, but maybe not in this case: transition period is only needed when there's no trivial workaround (and one that doesn't require 3rd party code to change).
The question is whether such trivial workaround exists in this case; as suggested here #10248 (comment) this can be used: nimble install oids although I haven't tested if it'd work if a user switches between nim devel and an older nim; probably would still work because I'm guessing stdlib has precedence of other nimble packages?

@narimiran
Copy link
Member Author

deserves a separate dedicated discussion in a github issue for easy cross-reference

---> https://github.com/nim-lang/RFCs

@Araq
Copy link
Member

Araq commented Jan 11, 2019

instead, it is derived from MongoDB concept of OID and still fails to ba compatible with the MongoDB spec

It was ported from C to Nim and then later MongoDB changed its spec / C implementation, that's why.

@narimiran narimiran deleted the graveyard branch January 21, 2019 13:37
@ITwrx
Copy link

ITwrx commented Jul 30, 2023

i object to the idea that smtp is not used much in the wild just because libraries on github.com don't need to send email. Nearly every web app will (contact forms, at the least) and some desktop apps will, and i'm guessing you wouldn't hear about it.

@Araq
Copy link
Member

Araq commented Jul 31, 2023

True but regardless of these facts Smtp as an external package not tied to Nim's slow release cycle makes sense.

@ITwrx
Copy link

ITwrx commented Jul 31, 2023

hmm, i hadn't thought about that aspect. I see that smtp is not even in the Graveyard repo (anymore?) and is in it's own repo. I guess modules in their own nim-lang repo are planned to be supported going forward. If so, great!

@Araq
Copy link
Member

Araq commented Jul 31, 2023

I guess modules in their own nim-lang repo are planned to be supported going forward. If so, great!

Yes, that is indeed the case and we try to ensure that the docs still cover these modules.

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.

6 participants