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 strutils.join for set type #23344

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

autumngray
Copy link
Contributor

Seems like an oversight not to have strutils.join for set types, so here is an implementation.

@juancarlospaco
Copy link
Collaborator

Should go in setutils https://nim-lang.github.io/Nim/setutils.html

@autumngray
Copy link
Contributor Author

Should go in setutils https://nim-lang.github.io/Nim/setutils.html

The procs in setutils all modify and/or produce sets whereas join produces a string, same as the other overloads in strutils. I think I would first try to import std/strutils when looking for a join():string. Putting it in setutils seems counterintuitive.

@metagn
Copy link
Collaborator

metagn commented Feb 23, 2024

Seems like an oversight not to have strutils.join for set types

join depends on the iteration order of collections, which is not guaranteed to be a specific order for sets and happens to be ascending order for Nim's bitset implementation. This isn't really a problem, but a version of join that allows an arbitrary iterator would be more useful so it's more explicit for the user, but there isn't a clean API for this without at least a template.

@Araq
Copy link
Member

Araq commented Mar 3, 2024

This makes no sense to me, when is $ for sets not sufficient?

@autumngray
Copy link
Contributor Author

To be specific, my use case was along these lines where I was expecting join to "just work":

type ApiParam = enum some allowed values
proc apiRequest(client: AsyncHttpClient, params: set[ApiParam]) =
  # params is a set, to constrain allowed api parameters. 
  let res = await client.get parseUri("https://example.com") ? {"paramset": params.join(",")}
  #...

Is this somehow to specific to go in std?

@Araq
Copy link
Member

Araq commented Mar 3, 2024

What about:

type ApiParam = enum some allowed values
proc apiRequest(client: AsyncHttpClient, params: set[ApiParam]) =
  # params is a set, to constrain allowed api parameters. 
  let res = await client.get parseUri("https://example.com") ? {"paramset": $params}
  #...

@autumngray
Copy link
Contributor Author

No, that would give e.g. "{a, b, c}" instead of "a,b,c". The api endpoint doesn't like the extra spaces+curly braces.

@Araq
Copy link
Member

Araq commented Mar 4, 2024

Still feels weirdly specific though as the order for set iteration is pretty arbitrary, as others noticed.

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.

None yet

4 participants