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

Design of functions taking optional names #156

Closed
daboross opened this issue Jul 2, 2019 · 2 comments · Fixed by #333
Closed

Design of functions taking optional names #156

daboross opened this issue Jul 2, 2019 · 2 comments · Fixed by #333
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup.

Comments

@daboross
Copy link
Collaborator

daboross commented Jul 2, 2019

The main four here are Room.createConstructionSite, Room.createFlag, RoomPosition.createConstructionSite and RoomPosition.createFlag.

Currently, neither createFlag can be called without a name - but this is something that the API should allow for. createConstructionSite is translated as two functions: create_construction_site and create_named_construction_site.

We should decide what pattern we want all of these to follow.

Possibilities I can think of:

  • one function taking Option<&str>
  • one function taking impl Into<Option<&str>>
  • two functions - one create_x, one create_named_x
  • two functions - one create_unnamed_x, one create_named_x
@daboross daboross added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-screeps-game-api labels Jul 2, 2019
@daboross
Copy link
Collaborator Author

daboross commented Jul 2, 2019

For create_construction_site, having a _named variant's nice since we rarely want to use the name. But I expect there to be codebases which use flags both ways (mainly for colors with no explicit name, and mainly for names)

@ASalvail
Copy link
Collaborator

ASalvail commented Jul 3, 2019

How close would you like to stick to the JS version?
If we want to stay as close as possible, then Option<&str> seems the best option (ha!).

I can imagine that the second option will be more concise, but I'm wondering if the template cost couldn't explode.

The last two are more in line with what we've been doing so far.

Another option, this one departing from the JS implementation, would be to start using more builder patterns to handle all of those cases. This would mean refactoring some existing function to keep it uniform, but might lead to a nicer API in the end.

So my vote goes for option 3 for consistency with current code, or completely new pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants