Skip to content

Enhance error message for invalid measure.vars in melt #6767

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

Closed
wants to merge 29 commits into from

Conversation

Divendra2006
Copy link

@Divendra2006 Divendra2006 commented Jan 26, 2025

Fixes #6512
It provides more informative error messages, better handling of edge cases, and an optimized, clean implementation. These changes enhance the overall user experience and make the function more robust in various use cases.

Benefits:

Improved Error Reporting: Users will receive more specific error messages indicating exactly which columns are invalid and need to be removed.

Better Performance:The function has been optimized for speed, particularly when dealing with large datasets.

Enhanced User Experience:The improved error messages and optimized performance lead to a better user experience when using the function in practical applications.

Testing and Validation:

Valid Inputs:The function works as expected when provided with valid column numbers, returning the unique columns.

Invalid Inputs:The function will raise an error with a detailed message if any columns are invalid, listing the problematic values.

NA_INTEGER Handling: When is_measure is true, NA_INTEGER is treated as a valid value, and the function behaves accordingly.

Edge Cases:The function has been tested with edge cases, such as empty inputs or all invalid columns, to ensure robustness.

Replace the message
"One or more values in 'measure.vars' is invalid" by
"One or more values in 'measure.vars' in invalid ; please fix by removing: %s"
it gives the specific details of error.

Copy link

codecov bot commented Jan 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.59%. Comparing base (d45546b) to head (650fe7a).
Report is 226 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6767   +/-   ##
=======================================
  Coverage   98.59%   98.59%           
=======================================
  Files          79       79           
  Lines       14664    14674   +10     
=======================================
+ Hits        14458    14468   +10     
  Misses        206      206           

☔ 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.

Copy link
Member

@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.

good start
please fix issues

@Divendra2006
Copy link
Author

@tdhock , Please let me know if there is any change or improvement needed .
If PR looks good I would be grateful if you could proceed with merging it at your earliest convenience.

Copy link
Member

@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.

when you push changes, please just push new commits instead of rewriting history and force pushing

@tdhock
Copy link
Member

tdhock commented Feb 20, 2025

Please click "Resolve conversation" when you have made changes to address reviewer comments.

Please add or edit test cases to reflect the new error messages.

@Divendra2006
Copy link
Author

@tdhock , there is some error when i use test case , can i close this PR??

@tdhock
Copy link
Member

tdhock commented Feb 22, 2025

I don't understand, can you please clarify?

@Divendra2006
Copy link
Author

One or more values in 'id.vars' are invalid; please fix by removing: [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0], [0 ] this type of error is coming ,

i don't know how to solve it .

@Divendra2006
Copy link
Author

can i make tests/testthat/test_melt.R file where we add test to reflect the new error message

@MichaelChirico
Copy link
Member

can i make tests/testthat/test_melt.R file where we add test to reflect the new error message

we do not use the testthat framework. See ?test, our unit tests will be in inst/tests/tests.Rraw

@aitap
Copy link
Contributor

aitap commented Feb 27, 2025

A merge conflict happens when commits on separate branches edit the same file in incompatible ways. For example, two commits both adding something at the end of a file is a conflict because it's unclear which edit should go first. Please undo 11345b9 because it reverts useful changes to tests made on master since your branch was started. (The overall diff is huge now, which shouldn't be happening.) Once that's done we can try to cleanly merge master into your branch. See also: advanced merging and conflict handling.

@Divendra2006
Copy link
Author

I resolve all conflict in vs code editor but here it shows confict.
can i open a new PR with new branch?

@aitap
Copy link
Contributor

aitap commented Feb 28, 2025 via email

@joshhwuu joshhwuu removed the request for review from a team March 6, 2025 21:36
@Divendra2006
Copy link
Author

Hey @joshhwuu, Just wanted to check in—are there any updates on the PR merge? Let me know if anything needs to be addressed. Thanks!

@Divendra2006
Copy link
Author

Hey @tdhock @aitap @joshhwuu
Please let me know if there is any change or improvement needed .
If PR looks good I would be grateful if you could proceed with merging it at your earliest convenience.

# test for enhancing error message of invalid column #6512
test(2308, {
melt(data.table(A = 1:5, B = 6:10), id.vars = c("A", "-1"))
}, error = "One or more values in 'id.vars' are invalid")
Copy link
Member

Choose a reason for hiding this comment

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

this test case is irrelevant.

please add test cases for the new functionality.

@venom1204
Copy link
Contributor

Hi Divendra, just checking in—has there been any progress on this?

@Divendra2006
Copy link
Author

Hi Divendra, just checking in—has there been any progress on this?

No, If you interested that you can

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.

melt error message for bad measure.vars could be more informative/specific
7 participants