Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix Log Entry to Config File Bug #406

Merged
merged 8 commits into from
Dec 17, 2024
Merged

Fix Log Entry to Config File Bug #406

merged 8 commits into from
Dec 17, 2024

Conversation

Lilferrit
Copy link
Contributor

Fixed a bug that would write the log entries to the config file instead of the log file when running the configure command.

@Lilferrit Lilferrit requested a review from bittremieux December 2, 2024 21:39
Copy link

codecov bot commented Dec 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.56%. Comparing base (17bb3f2) to head (b9d8b25).
Report is 1 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #406      +/-   ##
==========================================
- Coverage   94.61%   94.56%   -0.05%     
==========================================
  Files          14       14              
  Lines        1282     1289       +7     
==========================================
+ Hits         1213     1219       +6     
- Misses         69       70       +1     

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

Copy link
Collaborator

@bittremieux bittremieux left a comment

Choose a reason for hiding this comment

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

Looks good, just some considerations.

As you mentioned on Slack, I think it would also be good to add an integration test that checks whether the produced config file is actually compatible (i.e. you can run Casanovo with it).

casanovo/casanovo.py Outdated Show resolved Hide resolved
casanovo/casanovo.py Outdated Show resolved Hide resolved
@Lilferrit
Copy link
Contributor Author

Sounds good, I added additional test cases for the configure sub-command as well splitting the file IO options into a different class.

@Lilferrit Lilferrit requested a review from bittremieux December 6, 2024 01:57
@bittremieux bittremieux merged commit ea0a82c into dev Dec 17, 2024
4 of 5 checks passed
@bittremieux bittremieux deleted the config-fix branch December 17, 2024 19:23
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants