Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #667 +/- ##
=======================================
Coverage 98.79% 98.79%
=======================================
Files 40 40
Lines 2492 2492
=======================================
Hits 2462 2462
Misses 30 30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c4e738d to
e5065d4
Compare
2277562 to
04f32d9
Compare
|
No API break detected ✅ |
dotsdl
left a comment
There was a problem hiding this comment.
Thanks for the improvements to this how-to! I've found several critical technical issues that need fixing before merge. I'm also creating a companion PR with additional enhancements (Overview section, Best Practices, etc.) that you can reference or cherry-pick from.
Critical Fix 1: Wrong method names (Line ~90)CRITICAL: The method names are incorrect. Should be Current line says: These are the actual private method names that need to be implemented for serialization. If users follow this as written, their code won't work. Suggested fix: In other cases, such as when inheriting from ``ExplicitMoleculeComponent``, you will only need to define methods specifically not implemented - in this case ``_to_dict()`` and ``_from_dict()``. |
Critical Fix 2: Missing imports (Line ~45)CRITICAL: The first test example is missing required imports. As written, this code won't run because Current code is missing these imports: from gufe.tests import GufeTokenizableTestsMixin
import pytest
from rdkit import ChemSuggested fix: Add all imports at the top of the test file example in Step 2. |
Critical Fix 3: Missing serialization warning (After Step 4 example)CRITICAL: Need to warn users that adding custom attributes (like Suggested addition after the .. warning::
If you add custom attributes like ``custom_attribute`` above, you must also implement
``_to_dict()``, ``_from_dict()``, and ``_gufe_tokenize()`` methods to properly handle
serialization and hashing. See the :ref:`serialization how-to <howto-serialization>`
for details. The example above is simplified to demonstrate adding methods; in practice,
you may want to add only methods (not attributes) to avoid serialization complexity. |
Critical Fix 4: Broken Sphinx reference (Line 6)The Sphinx reference syntax is malformed - missing space in Current: The **gufe** :ref:`Component<component>` is a GufeTokenizable intended to be used...Should be: The **gufe** :ref:`Component <component>` is a :ref:`GufeTokenizable <understanding_gufetokenizables>` intended to be used as an *extensible point* of the library, so that you can describe a simulated system in terms of ``GufeTokenizable``\s that are compatible with **gufe** and the rest of the OpenFE ecosystem.This ensures proper cross-linking in the rendered documentation. |
Critical Fix 5: Missing news entryPR checklist requires a news entry but none exists. Please create **Added:**
* Added how-to guide for defining custom ``Component`` subclasses, covering extensible base classes, required methods, testing strategies, and serialization considerations.
**Changed:**
* <news item>
**Deprecated:**
* <news item>
**Removed:**
* <news item>
**Fixed:**
* <news item>
**Security:**
* <news item> |
resolves #521
Tips
Since this will create a commit, it is best to make this comment when you are finished with your work.
Checklist
newsentryDevelopers certificate of origin