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

Constructors taking Into<String> #16

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

icefoxen
Copy link
Contributor

@icefoxen icefoxen commented Jun 27, 2022

Description

Adds various new() funcs taking Into<String>

This is basically just boilerplate, as discussed in #15. This is in its own PR so you can decide separately whether or not you want it. It's not really finished, just intended as a sample for discussion.

Changes proposed in this pull request

  • Add new() functions for most/all types so that we can take Into<String> for names instead of just Strings.

ToDo

Still need to do for this:

  • Add functions for Instr struct
  • Proposed feature/fix is sufficiently tested
  • Proposed feature/fix is sufficiently documented
  • The "Unreleased" section in the changelog has been updated, if applicable

icefoxen added 4 commits June 25, 2022 23:21
Doesn't help quite as much as one might hope because there's
still lots of structures that you build literally, and those all
contain `String` and kind of have to.  If you want me to make
constructor functions for those I will, but that's an API decision
I happily leave up to you.
Basically just boilerplate, as discussed in garritfra#15.  This is in
its own PR so you can decide separately whether or not you
want it.

Still need to do for this:

 * Instr struct
 * Docstrings
 * Unit tests?
@garritfra
Copy link
Owner

From your ToDo-list I read that you're still working on this PR. I'll convert this to a draft, for clarity. Hope you don't mind! 🙂

@garritfra garritfra marked this pull request as draft June 28, 2022 06:37
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.

2 participants