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
Version 1.0.3 #8
Version 1.0.3 #8
Changes from all commits
dd51440
44ae78d
179b069
cd11a6d
855b5c4
ebf6e68
58c916d
92c164f
8ed9fc0
cc8997b
08b3209
0c235d1
8c27acb
261825e
7c33179
59fbaa9
c34f546
e01cbaa
7110f79
0d945a6
3903d90
a85f707
cc39ef9
a73755d
31c2856
9f6d3ad
0b9fbca
18e790b
172859f
eb4b017
88ae972
d4ee3cd
dec6e5c
7189a90
ac4776b
20877fd
bc34ae7
612fd42
2971bb2
89a41b1
638e6ec
ca035a4
a4e4e76
1605625
789a9a0
43f8c71
a61d882
a0d9104
b8c481a
1fdc9ee
a593148
a79cfbb
5b531fa
72e8139
073a04e
43aae90
780581d
0e393f3
1f44270
fc4a58c
e2ea20b
75e8264
74571ce
fca578e
f9da599
aa4d709
c73ea7b
3c4b4de
efca44d
2b85313
dbb4959
a10af51
02a85ae
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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.
Shouldn't be
os.path.dirname(exe)
?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.
Yeah sorry, the code for that is a little messy over the multiple classes. Basically, that line is for Mac when the subclass returns the path to
Minecraft.app
. The thing is that's both the executable and the folder! That's why it's necessary but I could probably clean it up a little.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.
Ou, I see. In this case the best would be to inject OS implementation to LocalClient (composition) and expose only higher level methods in LocalClient (like
run
or whatever you need there). Or, to avoid refactor just put some "if" statements. It is better to have more code but explicite.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.
every folder is executable BTW. "Running" .app folders on Mac is just a syntastic sugar for system layer to run proper executable in
x.app/Contents/MacOS
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.
Yes, I definitely need to refactor local client to be better, in fact I kind of started halfway with the
launch
functions but then stopped because it was working. I'll work on it anyway :)Absolutely did not know that, thanks a lot for that info.
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 is hard to follow this logic. You may consider refactor in the future to make it more maintainable. I would limit purposes and simplify the method interface, or at least document it /cover with tests.
But for now, better do not change anything if it works :) (I see that it is used in many places around the code)
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.
Yeah I've touched on the badness of this code in this reply to one of your other comments (here). Basically you are completely correct. It does work so I'm not touching it but definitely it would be better if a refactor it in the future.
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.
Just a tip for the future: follow single responsibility rule. Keep logic in relevant layer. Do simple interfaces: single method that do single simple thing like:
find_launcher_dir()
(without arguments that modify the inner logic). Such method should return always path to launcher and that's it. Then you can createfind_executable(directory, [game_id](if really needed))
to get exact executable andrun()
method separate for Mac and Windows that utilize other methods under the hood.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.
Thanks that's very true, I will be doing some heavy refactoring in local.py
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.
Please do not refactor until we merge current working state :)