Closed JosephAbbey closed 2 years ago
I appreciate your contribution and initiative in tackling so many issues!
Unfortunately, I can't accept this pull request as is. There are too many changes to review at once. Also, I've noticed a few issues:
The commit history is a bit disorganized. There are spurious commits with small changes, which should ideally be squashed together (for example, through git rebase
). There are also commits without any commit message at all.
There have been some file changes which I can't see good reasons for, such as deleting .gitattributes
and .github/FUNDING.yml
, or adding SECURITY.md
and .github/dependabot.yml
.
Many links have been redirected to your own forked repository.
The addition of htmlCollector.js
, in particular, is very difficult to review. Did you write it yourself? What does it do exactly? Isn't there a node.js library we can use instead, to keep the repository simple? If it's impossible to avoid, I think I'd rather not have it (and its associated features) included in the repository.
There's been the inclusion of a code coverage service, which I think should be discussed totally separately.
About the new GitHub Actions build targets, could we split them up? So that each build target is inside its own .yml file. We'd have one .yml workflow for Windows, one for Linux, and so on. I'm not really sure what the best practices are in this situation, but I feel that the workflows would be easier to review.
Still about the new build targets, there appears to have been a change to the structure of the output .zip file. Does it still include the readme and example files on all platforms?
There appears to have been a change to the webpage address, which perhaps is not such a good idea? Since it's been up for many years now.
About the vergen
dependency specifically... I was considering removing it altogether, to simplify the build process.
I would like to kindly ask you to submit each new feature as its own pull request, if possible! Ideally, one pull request for each of the following changes (usually with one commit each):
That way I would be able to understand better what kinds of changes I'd be bringing into the repository. Do you think you can start by opening a new pull request for the new build targets alone?
Sure thing,
The many commits were mainly Todo with getting the GitHub Actions workflow to work, just minor tweaks per commit.
The HTMLCollector.js script is to generate a portable version of the web build, which is necessary to use on some work networks that have firewalls.
Without vergen you will need some other way of tracking versions into rust.
The release zip file contents and the web path can be reverted to how they were.
The dependabot.yml file is for GitHub dependabot which keeps dependencies upto date. The security.md file is purely to show which versions are still supported.
As far as the GitHub Action it does need to be in one file so that it only releases after all the builds are done and it only builds after the tests have passed.
I agree that code coverage should be tackled separately.
I'll add back your .gitattributes file and your FUNDING.yml and I'll make sure that all of the links point to your repo.
I will create a new fork with one commit per change and after all the commits one tag for a minor version bump.
I have now created a new pull request with the discussed changes. #152.
I have changed the build process to asyncronousely build all platforms (windows, linux, web, web-portable) then publish to github-pages and github releases. You will no longer need a
ghpages
branch as I make use of the new "Deploy GitHub Pages site" action (https://github.com/marketplace/actions/deploy-github-pages-site).I also added the "Hex Line by Line" (1 instruction of hex per line) and the "Bin Line by Line" (1 instruction of binary per line) formats.
I have made it possible for the linux version to run on CentOS 7 (which many old build servers still run).
I have generated a portable (1 file, no internet) version of the web build.
I have made the line references in the error output link to the line in the code.