Skip to content

Feature: Better config modularity & alignment to nf-core config #268

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

Draft
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

JosuaCarl
Copy link
Collaborator

@JosuaCarl JosuaCarl commented Jul 18, 2025

Related Issues

  • Files are currently only given with {traceFile: path/to/file.txt}, whereas nf-core uses {trace: [file: path/to/file.txt]}

🎯 Motivation

  • Align plugin config with nf-core template
  • Improve modularity and checking during config creation and calling

📋 Summary of changes

  • Added BaseConfig class for configurations to make repetitive calls upon methods in a config easier
  • Added ConfigEntry to define entries of our config with desired attributes, including dynamic default initialization, type checking on every value change, nested Configurations setting, initialization indication
  • Added config classes for base file type and each specific file
  • Removed CO2Footprint from naming of nested classes, to avoid redundancy and long names

📌 Important details

  • Left originally configurable file parameters (overwrite & maxTasks) configurable for the respective files

✅ Checklist

  • New functionalities are covered by tests
  • Class structure in test reflects class structure in main
  • Documentation reflects changed behaviour
  • README.md contains information on or reference to new features
  • CHANGELOG.md is updated with a note on your changes
  • Ensure all tests pass (.\gradlew check)

JosuaCarl added 13 commits July 17, 2025 13:52
… conditions in static

Test: Added tests to new classes
Signed-off-by: JosuaCarl <[email protected]>
Test: Added tests for file configs
Signed-off-by: JosuaCarl <[email protected]>
Feature: Added additional file options

Test: Adjusted test for CO2FootprintConfig integration of trace, summary, report
Signed-off-by: JosuaCarl <[email protected]>
Test: Adjusted Tests to new config
Signed-off-by: JosuaCarl <[email protected]>
@JosuaCarl JosuaCarl requested review from skrakau and nadnein July 18, 2025 14:09
@JosuaCarl JosuaCarl self-assigned this Jul 18, 2025
@JosuaCarl JosuaCarl added 📝 documentation Improvements or additions to documentation 💎 enhancement New feature or request 🔀 refactor Change things to leave it the same 🐘 big A lot of changed code 🗄️ output Something surrounding the output (files) labels Jul 18, 2025
@JosuaCarl JosuaCarl marked this pull request as ready for review July 21, 2025 10:23
Copy link
Collaborator

@nadnein nadnein left a comment

Choose a reason for hiding this comment

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

Comments I have so far, I will continue tomorrow, it's a loooot :D

Comment on lines +102 to +103
Double getCi() { evaluate('ci') }
Double getCiMarket() { evaluate('ciMarket') }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Double getCi() { evaluate('ci') }
Double getCiMarket() { evaluate('ciMarket') }
/**
* Returns the carbon intensity value.
* If set as a closure (for real-time API), invokes it to get the current value.
*/
Double getCi() { evaluate('ci') }
/**
* Returns the personal energy mix carbon intensity value.
* If set as a closure (in case user defined a function for it in the config), invokes it to get the current value.
*/
Double getCiMarket() { evaluate('ciMarket') }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I intentionally deleted the comments, because the functionality should be clear form the commented evaluate function. I would rather modify this and keep the flow of the getmethods.

Comment on lines +138 to +149
String toString() {
return "${name}: ${value} (${allowedTypes} -> ${returnType})${description ? '\n' + description: ''}"
}

/**
* A Map representation of the parameter.
* @return Map representation
*/
Map<String, ?> toMap() {
return [name: name, value:value, allowedTypes: allowedTypes, returnType: returnType, description: description]
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are those functions needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

toString is mostly for debugging. The debugger calls the method to describe the instance. I found it really helpful and would definitely keep this.
toMap was intended for easier testing. Although I think I did not end up using it. But then again, I could also expand on the test of the class.


@Slf4j
class ConfigParameters {
final private HashMap<String, ConfigEntry> vault
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm shouldn't variable names be as descriptive as possible and not have vague names? And storage or vault is super generic and not descriptive for that variable

@JosuaCarl JosuaCarl requested a review from nadnein July 22, 2025 13:18
@nextflow-io nextflow-io locked as off-topic and limited conversation to collaborators Jul 24, 2025
@JosuaCarl JosuaCarl marked this pull request as draft July 24, 2025 13:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🐘 big A lot of changed code 📝 documentation Improvements or additions to documentation 💎 enhancement New feature or request 🗄️ output Something surrounding the output (files) 🔀 refactor Change things to leave it the same
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants