Refactor client code and extract repository/library methods into new modules #510
Conversation
tobil4sk
changed the title
tobil4sk
force-pushed
the
repo-refactor
branch
from
36fd95f
to
08d1628
Compare
tobil4sk
force-pushed
the
repo-refactor
branch
from
08d1628
to
f6052b7
Compare
tobil4sk
force-pushed
the
repo-refactor
branch
8 times, most recently
from
5124a38
to
26ec7cf
Compare
|
Pretty close to being done now, all that's left is to address #465 (because right now it's worse than it was before...) and to add some more tests for the new modules. I'll also go through and write down the full list of changes if anything was fixed/changed while rewriting the code. |
tobil4sk
force-pushed
the
repo-refactor
branch
from
26ec7cf
to
86f166a
Compare
|
Alright, so I've done all of the integration tests to cover the fixes I've made, and now I just want to write some more tests for the API by itself. Might be good to also consider how usable the API is in its current state as well. There were some bugs/issues that got solved by rewriting the code, and so instead of reintroducing the bug for the sake of it I documented everything that got fixed by rewriting the code and I wrote new tests for most of these changes. There was also #529 which after rewriting the code was left completely broken, so instead of recreating the intricate buggy state that we had before, I attempted to fix it. I will follow up with a summary of all the changes. |
|
Summary of changes: GENERAL
API
RUN
PATH
PATH and LIBPATH
LIST
INSTALL
SET
UPDATE
REMOVE
DEV, GIT and HG
|
tobil4sk
force-pushed
the
repo-refactor
branch
from
86f166a
to
dad9398
Compare
Generate the regex from TARGETS to avoid repetition
`-D run` requires hashlink library to be installed even for bytecode compilation.
| err: Std.string(e) | ||
| } | ||
| } | ||
| final out = p.stdout.readAll().toString(); |
There was a problem hiding this comment.
For the future: This is not guaranteed to work because stderr could be full before stdout finishes reading and vice versa. This is actually a really annoying problem with our Process API and the only way around it is to use threads to make sure the pipes don't clog. We should at some point look into this situation, even if in practice it is unlikely to make much of a difference.
There was a problem hiding this comment.
Would it be fixed if the exitCode() call comes before those two?
There was a problem hiding this comment.
No, the process still won't terminate if its pipes are full. Calling exitCode would trigger a flush, but that doesn't help if nobody picks anything up on the other side.
| } | ||
| } | ||
|
|
||
| private class AlreadyUpToDate extends InstallationException {} |
There was a problem hiding this comment.
We can keep this for now, but in general I would prefer not to manage control flow via exceptions. I'm pretty used to it in OCaml, but it can be a mess even there and we really shouldn't have to do this in Haxe.
It is now combined with installFromHaxelib, whose version argument is now optional.
No need to check if the requested version matches the scope version, as if that were the case it would have been caught in the check above.
Prevent reinstalling existing libraries
Reverting to previous behaviour
|
Happy birthday, PR #510! |
|
🥳🥳 |
This PR aims to make the client code cleaner and put it into modules that might be reusable in other tools, such as IDEs, or other frontends to the compiler.
Right now the
haxelib.client.Mainclass is where most of the client code is found. Separating the specific user interface elements of the haxelib client from the functionality behind managing the database would be a step towards making the haxe frontend independent of haxelib. It would be possible to just reuse these specific modules to fetch the library locations from where the haxelib client installs them.Other more general changes are usingMoved to #547finalwhere possible to keep code clean, and making use of more modern haxe 4 features.Progress:
Issues fixed: