Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Core: move PlandoConnections and PlandoTexts to the options system #2904
Core: move PlandoConnections and PlandoTexts to the options system #2904
Changes from 15 commits
c2be561
aab79bb
9ec268d
226932e
1f43d4d
913a49a
fea2a34
184129a
6fee5a2
5571fc5
51cda33
19b9c03
7b69879
1b88395
65bc9bf
6a3b982
e0bf453
ed582ae
4cf6616
c8a0e53
a76b008
7859129
194de1b
8d3366f
3ca1179
3fb3c3e
8086f0b
b840d15
a0d3002
b0f5dea
9737715
aa894a0
4c5f409
0ad6af1
39284e1
1e4a2f8
0591b2c
9ae1db0
8bbbd86
a96c57f
d3fc611
b27d71d
3a1bcff
2ad1270
ddc8b00
f5c2dc4
af84aef
ea37f2a
9a2593a
e6eac0c
3d2bde4
22e4d59
3b1dbd9
6af6ba2
d3dfb96
1edf701
8fad93b
2991ea2
a18e668
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is going to be a scoped class like this it should probably be in the PlandoConnection one instead of here so that the direction can be correctly typed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with this.
But also, maybe an
Enum
?(The downside to making it an
Enum
is the long definition...PlandoConnection.Direction.both
compared to"both"
)So another alternative is no class, just a type alias:
(I think a type alias has to be gloabal scope.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once 3.8 is dropped, StrEnum would probably be a good option. Not sure what the best option is for right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see what value
StrEnum
would have overEnum
for this.The values in a normal
Enum
can bestr
.StrEnum
is for when you want to pass it to a function that only takesstr
, or usestr
methods likelower
, but I don't see you doing any of that here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It almost seems better to make the values:
so that you could use those values in
get_option_name
(and simplify the code there)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the issue with that is that the existing implementations specifically compare the string when resolving the connections, so it'd need to be an alias of some sort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I didn't realize this was connecting with past implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we sure this should be the default behavior? it seems like enough of the worlds that use this would be fine with it (and my world is too) but it almost feels backwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It primarily does two things.
As you've said, it's sufficient for worlds that don't have wild constraints on possible connections. It also simplifies backward compatibility with the currently implemented games, as they already have to check for whether or not the plando is a valid connection, so we can skip the option-based check (the exception being Minecraft, as it's can_connect was simple enough to implement on the option itself).