API/UI Layout and Refactor

[arcanericky]

There are some pretty large changes to the code with this PR, so I can understand why you may not accept them. This project is getting large enough that all the code should not be in a main package anymore. These changes set the stage for easier code maintenance and eventual unit tests.

I’ve only tested using the examples from the README. Maybe these changes will help open up the code for unit tests for us to make changes and feel more confident in them.

To summarize these changes:

• Removed the dashes (-) from the package name as in go.mod. These dashes make it impossible to use the package (not allowed in package declarations). Eventually the project name should be changed if other projects wish to import this API.
• Begin separating user interface code from API code.
• Create a cmd/ directory for the UI code. This code handles command line options and environment variables, passing them to the API code that shouldn’t be bothered with these things.
• Updated makefile to handle this change above change
• This sets the stage for using a more robust CLI package such as spf13/cobra
• Extract code that can test and run in isolation into packages in the internal/ directory
  • routes/
  • targz/
  • zip/
• Move many of the global configuration variables into a httpfileserver.Config struct for now, populating the struct in the UI and passing it into the API. With this change almost all global variables have been removed and the configuration is injected via parameter.
• The version wasn’t being injected properly. Added a version variable for injection with logging at startup.
  Refactor to remove the init function which is now configureRuntime. init isn’t really unit testable.
• Chrome keeps trying to translate the page from Norwegian to English for me. Since there’s really nothing that must be translated on these pages, added <meta name="google" content="notranslate"/> to the HTML template to prevent this.

Having the code divided into these smaller packages with a UI/API separation makes it much more approachable by other contributors and unit tests could add another level of safety net for them. I’ve intentionally left much of the internal code and logic as is to keep this PR mainly as a repository layout change so if I’ve introduced defects, they can be more easily spotted.

I’ve noted there are no external dependencies - it’s a full standard library implementation. Is this intended, or are you open to using other packages such as spf13/cobra for the CLI, uber/zap for logging mholt/archiver for archive generation, etc?

There are no unit tests and no continuous integration (CI) as in GitHub Actions. Are you interested in having things such as lint, vet, fmt, etc run against the code on pull requests as well as adding unit test code?

Realize that by accepting this PR and then renaming your project to remove the dashes, you are making it easy for other projects to use the API code as an external package. This has “contract” implications that your exported functions (NewConfig(), Serve() and to an extent, the exported Config struct) in that changing the interface in the future is a breaking change, and the major version should be incremented each time this is done. I’ve tried to prevent this from happening by including a context.Context as the first parameter to Server() and placing all the config values into the Config struct. While removing a field from that struct would break the contract, adding a field would not as long as it is safely defaulted in the API code. I’ve made an attempt to be more flexible here by providing a NewConfig() function that can contain defaults in the future. Populating the defaults make using the API easier for the first time user. For example, the code:

httpfileserver.Serve(context.Background(), httpfileserver.NewConfig())

is enough to get the file server up and running since the proper fields are populated by NewConfig().

Another option is to move the code files httpfileserver.go and server.go into a directory and package in internal/ so it is no longer exported to other projects. Once you feel things have stabilized, move them back out into the project root directory at which point your initial contract is set. I didn’t include this change in the PR because I felt this PR would be more off-putting than it already is. I can make this change and add it to the PR if that’s desirable.

Even if you don’t accept this PR, let me know your thoughts. I enjoyed the refactoring challenges anyway.

[sgreben]

Wow! First off, thank you for the thoughtful PR and changes :) I'm busy with $work at the moment, so it will be about a week before I get around to doing a proper PR review. For now, a few high-level comments -

• Definitely agree on making the code easier to contribute to. In terms of code organization, I think it would be particularly nice to make the user-facing parts (CLI and HTML) easy to change/add to. For example, if someone wanted to add file search, it should be as obvious as possible where to find the appropriate "injection points". Or even simpler - adding custom CSS. This should ideally be a straightforward and "surgical" change.
• Also no objections to CI and testing automation.
• Re: libraries - fine in principle. Here my idea of a sweet spot is "vanilla Go code you can easily understand/change soon after learning the language, but also avoids fragile re-implementation of complex logic for which well-tested libraries already exist".
• Re:modules - is there a need to import http-file-server as a library? I haven't seen it so far - do you have any examples?

Thanks again for taking the time to assess the current state and design helpful changes! I'll give you detailed feedback on the diff & get this merged as soon as I can context-switch to github code :)

[arcanericky]

This project was useful to me for getting access to some files. I saw some opportunities here and thought I'd try to see what I could do and give back. I wouldn't make any feature changes without talking to you first. However, these changes set the stage for coordinating future modifications.

Before merging into your master branch, you might want to consider creating a develop branch for piling work together, coordination, and testing. Then once stable, merge into master for release.

Next up, I can put together a GitHub Action that will run fmt, vet, unit tests, build, etc to help validate pull requests. Then after that, start building unit tests bit by bit and as new code is modified or extracted. For example, I already have unit tests that cover all the Routes class.

I agree with most of your bullet points, but the last one (modules) is more of a question that I'll try to answer.

Even internally, having a project separated into packages is a good thing. It gives a separation of concerns and coordinates an internal contract between chunks of functionality. For example, setting up the internal Zip packages gives us a single entry point contract to the zip functionality. We can swap out the code behind that and as long as the API signature never changes, any other code that uses it would never know. Yes, you can also do that with a straight function as you have done, but a package truly locks things down so a user can't get at anything internal while providing you more flexibility in that package (package globals, introducing classes, etc).

The package httpfileserver should exist for the same reason. Having it helps provide an extremely clean separation between the UI and the package's API. You can skin the UI however you please and it can always call the httpfileserver API knowing the signature will never change. Your big decision here is whether you want to keep this httpfileserver internal and for your use only, or export it in a way that other projects can use it. I'll give examples for both. Remember these examples are contrived, but hopefully it's enough for you to see the bigger picture.

For this project's internal use. Suppose you like your current command line interface and want to keep things as is for this CLI executable. But one day you decide you'd like to create a GUI for the functionality using something like Fyne. You could split the cmd directory to contain a main package in the cmd/cli directory, and another main package in the cmd/gui directory. Each calls the httpfileserver API, each is a separate executable. The CLI executable doesn't have to carry all the GUI luggage, keeping it smaller, while the GUI package can import all that GUI code for those that really need it.

For external use. Suppose I'm a random user that has a need for a program like this. I find it here on GitHub, download and run it. It's exactly what I need except I need to hand it to a user where running it from the command line is challenging enough, but throw in all those awesome CLI options and mind blown. But because you've exported your core functionality as a package, I can solve the problem. I spin up a new project. Hardcode all the options to force serving from port 80 and to serve that user's /home/user/shareddata directory. And I don't want them exposing the upload functionality so the code forces that off. Now I can compile and hand that executable to my user knowing they can't jack up the configuration because I've hardcoded it. Bonus benefit: While your project isn't being consumed as an executable, you're still gaining a new user because you were forward thinking enough to provide an API. And more users at this level means those users are real developers that are even more likely to contribute to your code base than those that simply pull down your executable and run it.

Another external example. I like the executable and it's useful. However, the executable only allows listening on a single endpoint. I want something that listens on both http port 80 and https port 443. By spinning up my own project and using yours as an API, I can code something to do that. I can launch httpfileserver.Serve() in a goroutine to serve the http endpoint, then call httpfileserver.Serve() again to serve the TLS endpoint. Problem solved because you provided your code as an API. And once again, you've obtained another developer user.

Hopefully this helps you if you still have questions or concerns, then ask away. If you really don't want to export this as an API, we should move it to the internal directory, for example, internal/httpfileserver, which is the correct way to hide it.

I know I'm suddenly barging into this project. Thanks for being ok with it so far. I like that it strikes a balance between useful functionality and bloat. And that I can spot some areas to contribute.