Some refactoring, tests, doc blocks#25
Closed
splitbrain wants to merge 10 commits into
Closed
Conversation
This introduces an Interface that defines what a suitable HTTPClient needs to provide and makes it possible to pass an alternative implementation in the Lescript constructor.
Users may want to modify some behaviour of the library in sub classes. Making the inner workings accessible make that easier.
This implements the basic foundations for unit tests to LeScript. It uses the PHP builtin webserver and ngrok for having publically accessible domains that LeScript can work on.
* master: Example for #12 - adding LE registration contact Fix analogic#18 - make HTTP client exchangable Fix analogic#19 - Allow Defining challenge type Add #12 - adding LE registration contact
* protected: Made all private methods/fields protected
This makes it easier to maintain. A build script to create a single file again could be added if wanted.
the methods are still rather large, but this was the simples possible split for now. It might be more sensible for $accountDetails to be a member variable.
Contributor
|
This is really impressive and should be merged. 👍 One note: Does this require a major version bump? I suppose not. It looks like it's backwards compatible. |
|
Would you mind merging this? |
|
This should be merged. Is this repo managed still? |
Owner
|
I appreciate this pull request but it is far from the original purpose of Lescript - to be a simple single file without deps. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
This refactors the library into separate files per class as suggested by @tm1000 in #18. It also introduces unit testing with some initial tests. More tests at a deeper level would make sense though (currently it just checks the overall outcome). I also added doc blocks to make things bit easier to understand and for better type hinting in IDEs. This also incorporates #23. After merging, automated unit testing at travis-ci should be set up.
If you're not interested in this, feel free to close.