refactor(DigestionEnzyme): move sense-based constructor to DigestionE… by TheClaervoyant · Pull Request #174 · cbielow/OpenMS

TheClaervoyant · 2026-05-27T19:24:29Z

…nzymeProtein, danke Git
Ich habe mein gesamtes Repo gesmited und neu gebuilded und die Files dann nachträglich meine Files gemoved. Should work now

Description

Checklist

Make sure that you are listed in the AUTHORS file
Add relevant changes and new features to the CHANGELOG file
I have commented my code, particularly in hard-to-understand areas
New and existing unit tests pass locally with my changes
Updated or added python bindings for changed or new classes (Tick if no updates were necessary.)

How can I get additional information on failed tests during CI

Click to expand

If your PR is failing you can check out

The details of the action statuses at the end of the PR or the "Checks" tab.
http://cdash.seqan.de/index.php?project=OpenMS and look for your PR. Use the "Show filters" capability on the top right to search for your PR number.
If you click in the column that lists the failed tests you will get detailed error messages.

Advanced commands (admins / reviewer only)

Click to expand

/reformat (experimental) applies the clang-format style changes as additional commit. Note: your branch must have a different name (e.g., yourrepo:feature/XYZ) than the receiving branch (e.g., OpenMS:develop). Otherwise, reformat fails to push.
setting the label "NoJenkins" will skip tests for this PR on jenkins (saves resources e.g., on edits that do not affect tests)
commenting with rebuild jenkins will retrigger Jenkins-based CI builds

⚠️ Note: Once you opened a PR try to minimize the number of pushes to it as every push will trigger CI (automated builds and test) and is rather heavy on our infrastructure (e.g., if several pushes per day are performed).

…nzymeProtein

github-actions · 2026-05-27T19:24:42Z

Welcome @TheClaervoyant and thank you for your contribution to OpenMS! To help us better understand our userbase and contributors, we would appreciate if you could complete the survey at the link below. Completing this survey will help us get to your PR sooner. Thank you for your participation!

Survey

cbielow

nice change.
some minor things to consider.

cbielow · 2026-05-28T07:44:23Z

-    //@{
+   //@{
+    /// default constructor
+    DigestionEnzyme();


why make this public?
This default enzyme is not functional.

Changed it to protected. DigestionEnzyme Protein and DigestionEnzymeRNA need it. But to not access it from outside i made it protected

cbielow · 2026-05-28T07:44:53Z

  {
  public:
-
+    enum Sense {C_TERM,N_TERM};


Suggested change

enum Sense {C_TERM,N_TERM};

enum class Sense {C_TERM,N_TERM};

cbielow · 2026-05-28T07:47:24Z

+    explicit DigestionEnzymeProtein(const String& name,
+                             String cut_before,
+                             Sense sense,
+                             const String& nocut_after = "",
+                             const std::set<String>& synonyms = std::set<String>(),
+                             String regex_description = "");


make all Strings of the form "const String&". There is no point in moving, since this is not high performance code, and the strings are short, so SSO (small string optimization) should kick in anyway.

add @param comments. e.g. its important to understand that e.g. cut_before is treated as a set of amino acids where cutting can occur, and not as a string. e.g. trypsin would be 'KR', or 'RK' (order does not matter)

Done. I think. I removed move were I could

cbielow · 2026-05-28T07:52:37Z


    Int omssa_id_;

+    String buildRegex_(String& cut_before, const String& nocut_after,const DigestionEnzymeProtein::Sense& sense);


Suggested change

String buildRegex_(String& cut_before, const String& nocut_after,const DigestionEnzymeProtein::Sense& sense);

String buildRegex_(String cut_before, const String& nocut_after,const DigestionEnzymeProtein::Sense& sense);

you don't want to add 'X' and have that reflected in the caller.

Also adapted

cbielow · 2026-05-28T07:53:46Z

+  if (cut_before.empty())
+  {
+    throw Exception::MissingInformation(
+        __FILE__, __LINE__, OPENMS_PRETTY_FUNCTION,
+        "No cleavage position given when trying to construct a DigestionEnzyme.");
+  }
+
+  for(char c : cut_before)
+  {
+    if (c > 'Z' || c < 'A')
+    {
+      throw Exception::InvalidParameter(
+        __FILE__, __LINE__, OPENMS_PRETTY_FUNCTION,
+        "Amino Acids for cleavage contain unknown character: " + String(c));
+    }
+  }
+
+  for(char c : nocut_after)
+  {
+    if (c > 'Z' || c < 'A')
+    {
+      throw Exception::InvalidParameter(
+        __FILE__, __LINE__, OPENMS_PRETTY_FUNCTION,
+        "Amino Acids to stop cleavage contain unknown character: " + String(c));
+    }
+  }


document these exceptions in the constructor using

@throw Exception::InvalidParameter if ....

cbielow · 2026-05-28T08:47:55Z

+  TEST_EQUAL(enzyme.getSynonyms().size(),2)
+
+  // Test incorrect keys.
+  TEST_NOT_EQUAL(enzyme.setValueFromFile("test","Tryp-Like"),true)


Suggested change

TEST_NOT_EQUAL(enzyme.setValueFromFile("test","Tryp-Like"),true)

TEST_FALSE(enzyme.setValueFromFile("test","Tryp-Like"))

cbielow · 2026-05-28T08:48:10Z

+  TEST_EQUAL(enzyme == "Verify",true)
+  TEST_NOT_EQUAL(enzyme == "Accept",true)


cbielow · 2026-05-28T08:48:58Z

+  DigestionEnzyme enzyme;
+  enzyme.setName("TestEnzyme");


a default enzyme should not be allowed IMHO

I removed this Test. In order to initialise an empty Enzyme tho, I used DigestionEnzymeProtein, since this is possible there. I just test the base functions, however. Imo the best middleground between needed test and good readability.

cbielow · 2026-05-28T08:49:47Z

+    DigestionEnzymeProtein n_term_test("N-Term_Style", "D", DigestionEnzymeProtein::Sense::N_TERM, "E");
+    TEST_EQUAL(n_term_test.getRegEx(), "(?<![E])(?=[DX])")


add a test where 'X' is already present (but not as a suffix)

cbielow · 2026-05-28T08:50:38Z

+    DigestionEnzymeProtein with_x("AlreadyX", "KX", DigestionEnzymeProtein::Sense::C_TERM);
+    TEST_EQUAL(with_x.getRegEx(), "(?<=[KX])")


nice.
Add another one where 'X' is a prefix

I made both, I think, however, that they are redundant. We do not want an X other than as a Suffix, since an X anywhere would mean "This Protease can cut at undefined Enzymes" which practically includes everyone. Thus somehow rendering it useless. Both are included, but both will throw an Exception.

…DigestionEnzyme protected (classes like DEP and DERNA need it). Removed empty test from DigestionEnzyme_test. Used DigestionEnzymeProtein for Default Initialization. DigestionEnzymeProtein checks whether X is in cut_before. If its not a Suffix, throw Exception, since cutting anywhere is not a realistic Protease (X means any Amino Acid and cut_before is a SET as to where to cleave)

refactor(DigestionEnzyme): move sense-based constructor to DigestionE…

7a40baf

…nzymeProtein

github-actions Bot added the new contributor label May 27, 2026

cbielow requested changes May 28, 2026

View reviewed changes

	enum Sense {C_TERM,N_TERM};
	enum class Sense {C_TERM,N_TERM};


		Int omssa_id_;

		String buildRegex_(String& cut_before, const String& nocut_after,const DigestionEnzymeProtein::Sense& sense);

	String buildRegex_(String& cut_before, const String& nocut_after,const DigestionEnzymeProtein::Sense& sense);
	String buildRegex_(String cut_before, const String& nocut_after,const DigestionEnzymeProtein::Sense& sense);

	TEST_NOT_EQUAL(enzyme.setValueFromFile("test","Tryp-Like"),true)
	TEST_FALSE(enzyme.setValueFromFile("test","Tryp-Like"))

		TEST_EQUAL(enzyme == "Verify",true)
		TEST_NOT_EQUAL(enzyme == "Accept",true)

		DigestionEnzymeProtein n_term_test("N-Term_Style", "D", DigestionEnzymeProtein::Sense::N_TERM, "E");
		TEST_EQUAL(n_term_test.getRegEx(), "(?<![E])(?=[DX])")

		DigestionEnzymeProtein with_x("AlreadyX", "KX", DigestionEnzymeProtein::Sense::C_TERM);
		TEST_EQUAL(with_x.getRegEx(), "(?<=[KX])")

Conversation

TheClaervoyant commented May 27, 2026

Description

Checklist

How can I get additional information on failed tests during CI

Advanced commands (admins / reviewer only)

Uh oh!

github-actions Bot commented May 27, 2026

Uh oh!

cbielow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants