Skip to content

Conversation

@ANAMASGARD
Copy link
Contributor

@ANAMASGARD ANAMASGARD commented Oct 19, 2025

This PR adds two checks to prevent tabs from being introduced into inst/htmljs/animint.js:

  1. GitHub Actions workflow (.github/workflows/check-tabs.yml) - Fails CI if tabs are detected
  2. R test (tests/testthat/test-linter-no-tabs.R) - Tests during package checks

Testing

Tested locally by:

  1. Verified no tabs in source:

    grep -P '\t' inst/htmljs/animint.js
    # No output - no tabs found ✓
  2. Installed package and ran test (should pass):

    R CMD INSTALL .
    Rscript -e 'library(testthat); library(animint2); test_file("tests/testthat/test-linter-no-tabs.R")'
    # Result: PASS (2 tests passed) ✓
screen-pic-1
  1. Added tab to line 10 and tested (should fail):
    R CMD INSTALL .
    Rscript -e 'library(testthat); library(animint2); test_file("tests/testthat/test-linter-no-tabs.R")'
    # Result: FAIL - "Tab characters found in animint.js at lines: 10" ✓
pick-part-2
  1. Removed tab and verified test passes again:
    The test correctly detects when tabs are present and passes when they're removed.

Files Changed

  • .github/workflows/check-tabs.yml - New CI workflow
  • tests/testthat/test-linter-no-tabs.R - New R test

Fixes #256

Adds CI workflow and R test to catch tab characters.
JavaScript should use spaces, not tabs for indentation.

Fixes #256
@codecov
Copy link

codecov bot commented Oct 19, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.37%. Comparing base (efbe67b) to head (c756dd9).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master     #257       +/-   ##
===========================================
+ Coverage   77.71%   95.37%   +17.65%     
===========================================
  Files         164        1      -163     
  Lines        8788     2788     -6000     
  Branches      562      574       +12     
===========================================
- Hits         6830     2659     -4171     
+ Misses       1958      129     -1829     
Flag Coverage Δ
javascript 95.37% <ø> (ø)
r ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ANAMASGARD ANAMASGARD requested review from Copilot and tdhock and removed request for Copilot October 20, 2025 14:48
@@ -0,0 +1,20 @@
test_that("animint.js contains no tab characters", {
animint_js_path <- system.file("htmljs", "animint.js", package = "animint2")
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add mustWork=TRUE

@ANAMASGARD
Copy link
Contributor Author

ANAMASGARD commented Oct 21, 2025

Sir Thank a lot for the suggestion! I looked into using lintr as you recommended. However, lintr is specifically designed for linting R code, not JavaScript files.​

Since we're checking inst/htmljs/animint.js (a JavaScript file) for tabs, using lintr would require creating a complex custom linter that reads non-R files, which seems like overkill for this simple check.​

The current grep-based approach:-
1-) Works directly on the JavaScript file​.
2-) Requires no R dependencies​.
3-) Runs faster (no R setup needed)​.
4-) It is also easier to maintain​ .

Would you be okay with keeping the simple grep approach in .github/workflows/check-tabs.yml and removing the redundant R test file entirely as you mentioned ? This gives us the CI protection you want without unnecessary complexity.

The https://github.com/Rdatatable/data.table/blob/master/.github/workflows/code-quality.yaml
you mentioned kind of feels in my opinion overkill (as mentioned above) for checking of tabs in one single JavaScript file.
In Data.table workflow checks hundreds of R, C, markdown, and documentation files across an entire codebase.

Sir if you want to add some feedback or correct me if I am wrong in any way of my understanding.

@tdhock
Copy link
Collaborator

tdhock commented Oct 21, 2025

please avoid cross-posting (you wrote the same comment in #256 ).

can you please push a commit which adds a tab to animint.js, so we can see that the test would correctly fail in that case?
(and then push another commit which removes the tab)

Added a tab character to line 1 of animint.js to demonstrate
that the check-tabs workflow correctly detects and fails when
tabs are present.

This commit should result in a failed CI check.
Removed the tab character from line 1 of animint.js to demonstrate
that the check-tabs workflow correctly passes when no tabs are present.

This commit should result in a successful CI check (green checkmark).
@ANAMASGARD ANAMASGARD requested a review from tdhock October 21, 2025 16:35
Copy link
Collaborator

@tdhock tdhock left a comment

Choose a reason for hiding this comment

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

please remove test-linter-no-tabs.R

ANAMASGARD and others added 2 commits October 22, 2025 19:09
As requested by @tdhock, removing test-linter-no-tabs.R to avoid
redundancy. The GitHub Actions workflow check-tabs.yml already
provides the necessary linting for tabs in animint.js.
@ANAMASGARD
Copy link
Contributor Author

Root cause: The test test-compiler-ghpages.R tries to DELETE a GitHub repository but lacks permissions:

❌ R_coverage fails due to pre-existing GitHub API permissions issue

Sir should I proceed with this PR as-is, or would you like me to investigate the ghpages test issue separately in a different PR?

@tdhock
Copy link
Collaborator

tdhock commented Oct 22, 2025

when there is a ghpages test failure that is typically a false positive, which we can ignore.
or even better, restart the build on the action page.

@tdhock tdhock merged commit 94c2ae9 into master Oct 22, 2025
7 of 9 checks passed
@tdhock
Copy link
Collaborator

tdhock commented Oct 22, 2025

great thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add linter to ensure no tabs in animint.js

2 participants