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

Fix #62 #75

Merged
merged 2 commits into from
Feb 10, 2021
Merged

Fix #62 #75

merged 2 commits into from
Feb 10, 2021

Conversation

gabbhack
Copy link
Contributor

  • Supports more than one pragma parameter (returns tuple)
  • Returns nil if there are zero parameters in the pragma (like std)

- Supports more than one pragma parameter (return tuple)
- Returns nil if there are zero parameters in the pragma (like std)
@zah
Copy link
Contributor

zah commented Feb 10, 2021

getCustomPragmaFixed was introduced quite some time ago when getCustomPragma from the standard library had a number of issues preventing real-world usage within our libraries. Since then, I've helped the Nim team fix the issues and I don't see any reason for new users to depend on getCustomPragmaFixed. It's kept around only because we haven't invested the time to rework the code that was based on it. What is your use case and why can't you rely on getCustomPragma? To clarify further, instead of changing the API here, we are likely to just drop getCustomPragmaFixed in favor of the standard library.

@arnetheduck
Copy link
Member

@zah can we provide a compatibility shim so that anyone depending on the version in stew is redirected to the upstream one instead? we can mark it as deprecated or whatever, but if possible, we shouldn't unnecessary break user code

@gabbhack
Copy link
Contributor Author

@zah
I am writing another library for deserialization, and I ran into the problems already described in nim-lang/RFCs#176 (where I found this library):

@zah
Copy link
Contributor

zah commented Feb 10, 2021

I'm willing to accept this patch, but it would be a breaking change for a number of our internal packages that already use getCustomPragmaFixed:

https://github.com/search?q=org%3Astatus-im+getCustomPragmaFixed&type=code

Would you agree to take a look at the usages and submit suitable PRs for all projects?

@zah
Copy link
Contributor

zah commented Feb 10, 2021

Actually, I think I misunderstood the description and this is not really a breaking change. Apologies.

@zah zah merged commit 04f8150 into status-im:master Feb 10, 2021
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