Skip to content
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

fix(html): add html reporter to rockspec #103

Merged
merged 5 commits into from
Sep 7, 2023

Conversation

Aire-One
Copy link
Contributor

Hello 👋

Following my message in #98, here are the necessary changes to make the Html reporter work with the Luarocks package.

It adds a new optional dependency on datafile. Because it's optional (only required by the Html reporter), I haven't pushed it to the rockspec's dependencies table.

Alternatively, we could have included the file's content as long strings in the reporter.html.* module. Since the static files are larges, I think it's better to manage them as regular files in the repository and access them with datafile.

Fixes #98

Copy link
Member

@Tieske Tieske left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved the files and included datafile as a dependency. Thx for the fix

@Tieske Tieske changed the title Include the Html reporter to the Luarocks package fix(html): add html reporter to rockspec Sep 7, 2023
@Tieske Tieske merged commit d2c3f82 into lunarmodules:master Sep 7, 2023
@Aire-One Aire-One deleted the fix/#98 branch September 18, 2023 12:01
@Aire-One
Copy link
Contributor Author

Aire-One commented Sep 18, 2023

Thank you, @Tieske, for the merge!

I can see you removed the mention that the HTML Reporter depends on datafile. Should we consider as a good practice to change the local datafile = require("datafile") line to something like

local datafile_available, datafile = pcall(require, 'datafile')
if not datafile_available then
  -- print a message to inforn the user they need to install datafile, then exit the program
end

@Tieske
Copy link
Member

Tieske commented Sep 19, 2023

@Aire-One you're right, it is omitted. My intent was to add it to the rockspec as a dependency, but I see that I missed that.

Tieske added a commit that referenced this pull request Sep 19, 2023
@Tieske
Copy link
Member

Tieske commented Sep 19, 2023

see #108

Tieske added a commit that referenced this pull request Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Html reporter not working
2 participants