Refactors recipe conversion work #22

schuylermartin45 · 2024-04-08T15:20:44Z

Recipe conversion tooling is now broken-out into a subclass of the original RecipeParser
This keeps the `RecipeParser file from getting any bigger and will allow some future flexibility in maintaining the conversion process
Test files are broken up accordingly

There are no net logic changes, only code has been moved.

- Recipe conversion tooling is now broken-out into a subclass of the original `RecipeParser` - This keeps the `RecipeParser file from getting any bigger and will allow some future flexibility in maintaining the conversion process - Test files are broken up accordingly - Adds `conftest.py` to share testing utilities across multiple unit test files

jezdez

Just a small change in how you define these helpers, they would work well as fixtures with indirect parameters, or as utility functions living outside of the magic conftest module.

tests/parser/test_recipe_parser.py

jezdez · 2024-04-08T15:50:39Z

@schuylermartin45 I think the file is missing 🤗

schuylermartin45 · 2024-04-08T16:40:05Z

@schuylermartin45 I think the file is missing 🤗

This is what I get for quickly doing git add -u from muscle memory. Let me fix that. Weird that my local pre-commit checks didn't catch that uncommitted file.

jezdez

The tests seem to have trouble importing the new module, not sure why it wouldn't, maybe the indirection via make?

schuylermartin45 · 2024-04-08T19:34:17Z

I'm not sure if I follow, it looks like the only test failing is the rattler-build test that's probably a rattler issue.

marcelotrevisani · 2024-04-08T20:02:30Z

I'm not sure if I follow, it looks like the only test failing is the rattler-build test that's probably a rattler issue.

it is not really a problem with rattler, that is due the name path being too long
https://docs.python.org/3/library/errno.html#errno.ENAMETOOLONG

if you can try simply reduce the name of the file, that should do it

marcelotrevisani · 2024-04-08T20:06:41Z

tests/file_loading.py

+    return RecipeParser(recipe)
+
+
+def load_recipe_convert(file_name: str) -> RecipeParserConvert:


to load files for tests you can use pytest-datadir if you wish, that would simplify a few things

TIL, I'll have to look into that. I was not aware of that plugin.

marcelotrevisani · 2024-04-08T20:11:03Z

tests/file_loading.py

+        return f.read()
+
+
+def load_recipe(file_name: str) -> RecipeParser:


if you prefer you can use factories as fixtures here and remove the __init__.py and this bit here, and put everything in the conftest.py
example:
https://docs.pytest.org/en/7.1.x/how-to/fixtures.html#factories-as-fixtures

I appreciate and am aware, but I wanted to keep this PR smaller and focus refactoring one thing.

tests/file_loading.py

schuylermartin45 · 2024-04-08T21:07:49Z

I'm not sure if I follow, it looks like the only test failing is the rattler-build test that's probably a rattler issue.

it is not really a problem with rattler, that is due the name path being too long https://docs.python.org/3/library/errno.html#errno.ENAMETOOLONG

if you can try simply reduce the name of the file, that should do it

I've been pinning it on rattler as I think it is a change between versions. That path is not new and has worked in previous CI builds.
But again, I haven't had time today to investigate this further.

schuylermartin45 · 2024-04-08T21:24:30Z

I've been pinning it on rattler as I think it is a change between versions. That path is not new and has worked in previous CI builds. But again, I haven't had time today to investigate this further.

Well good news, I finally found time to look into :). From what I can tell, my initial gut feeling looks correct and I've filed an issue with rattler-build. Version 0.13.0 did not have an issue with this file path.

tests/file_loading.py

kenodegard · 2024-04-11T04:48:09Z

conda_recipe_manager/parser/recipe_parser_convert.py

+class RecipeParserConvert(RecipeParser):
+    """
+    Extension of the base RecipeParser class to enables upgrading recipes from the old to new format.
+    This was originally part of the RecipeParser class but was broken-out for easier maintenance.


How does breaking this out like this offer easier maintenance?

As far as I can tell the maintenance issue here is caused by the high cyclomatic complexity of the render_to_new_recipe_format method. A good rule of thumb is to aim for a complexity of 15 or lower but, as currently written, this method has a complexity of 49, much too high.

Furthermore if it is this easy to break this into a new class (and since self is only referenced once in the method) it would seem to me that this deserves to live as a function and not nested within a larger object:

def convert_to_new_recipe_format(old_recipe: RecipeParser) -> tuple[str, MessageTable]: new_recipe = RecipeParser(self.render()) ...

The subclass is mostly there to make the static analyzer happy with accessing underscore functions. I also have future plans to break this up into more functions under this new "namespace".

As for maintenance, the original parser class is huge. It used to be an even larger class file and was split-up into a few supporting files and classes.

Out of curiosity, how did you calculate that? Do you have a plugin? I also think some of this logic is unavoidable with the nature of the conversion work. There is a lot of "if this, then that" to perform.

Let me go and clean up a bit more here. I think I can have the message tracking and a few other pieces of state tracked as part of the class member that will simplify some of the ick.

I have McCabe linters enabled in my IDE. Since you're using pylint you can use https://pypi.org/project/pylint-mccabe/.

I just posted a few commits that clean-up the inner-function and state usage. If you're okay with it, I'd like to look into this mccabe integration after this PR and #29 get merged in. I think that might cause a bigger refactor

Co-authored-by: Ken Odegard <[email protected]>

- Moves state stored in `render_to_new_recipe_format()` to simplify logic - Moves inner-functions of `render_to_new_recipe_format()` to private members of the `RecipeParserConvert` subclass

schuylermartin45 · 2024-04-11T15:43:08Z

conda_recipe_manager/commands/convert.py

@@ -2,6 +2,7 @@
 File:           convert.py
 Description:    CLI for converting an old recipe file to the "new" format.
 """
+


black got an update in pre-commit the other day, hence the change here and in other files.

schuylermartin45 requested a review from a team April 8, 2024 15:20

jezdez requested changes Apr 8, 2024

View reviewed changes

tests/parser/test_recipe_parser.py Outdated Show resolved Hide resolved

Minor renaming

92f2755

schuylermartin45 requested a review from jezdez April 8, 2024 15:31

Forgot to add test file

249be19

jezdez reviewed Apr 8, 2024

View reviewed changes

marcelotrevisani reviewed Apr 8, 2024

View reviewed changes

tests/file_loading.py Outdated Show resolved Hide resolved

schuylermartin45 added 2 commits April 9, 2024 13:40

Test file path now uses a calculated absolute path

c0fbe0b

Changing rattler-build output dir in CI to test theory

0c1f9ab

schuylermartin45 requested a review from a team April 10, 2024 17:47

schuylermartin45 mentioned this pull request Apr 10, 2024

Refactor: Commands execute under conda-recipe-manager namespace #29

Merged

kenodegard reviewed Apr 11, 2024

View reviewed changes

schuylermartin45 and others added 5 commits April 11, 2024 08:31

Update tests/file_loading.py

8ef6643

Co-authored-by: Ken Odegard <[email protected]>

Update tests/file_loading.py

a307601

Co-authored-by: Ken Odegard <[email protected]>

Incorporates feedback on file reading

22df115

State refactor for render_to_new_recipe_format()

88b7a10

- Moves state stored in `render_to_new_recipe_format()` to simplify logic - Moves inner-functions of `render_to_new_recipe_format()` to private members of the `RecipeParserConvert` subclass

Marks RecipeParserConvert state as private

9d9f8b8

schuylermartin45 commented Apr 11, 2024

View reviewed changes

schuylermartin45 mentioned this pull request Apr 11, 2024

Ci test #27

Closed

schuylermartin45 requested a review from kenodegard April 11, 2024 16:10

schuylermartin45 mentioned this pull request Apr 11, 2024

Integration Testing, Round 2 #26

Merged

kenodegard approved these changes Apr 11, 2024

View reviewed changes

schuylermartin45 merged commit d011612 into main Apr 11, 2024
5 checks passed

schuylermartin45 deleted the smartin_refactor_conversion branch April 11, 2024 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactors recipe conversion work #22

Refactors recipe conversion work #22

schuylermartin45 commented Apr 8, 2024 •

edited

Loading

jezdez left a comment

jezdez commented Apr 8, 2024

schuylermartin45 commented Apr 8, 2024 •

edited

Loading

jezdez left a comment

schuylermartin45 commented Apr 8, 2024

marcelotrevisani commented Apr 8, 2024

marcelotrevisani Apr 8, 2024

schuylermartin45 Apr 8, 2024

marcelotrevisani Apr 8, 2024

schuylermartin45 Apr 8, 2024

schuylermartin45 commented Apr 8, 2024 •

edited

Loading

schuylermartin45 commented Apr 8, 2024

kenodegard Apr 11, 2024

schuylermartin45 Apr 11, 2024

schuylermartin45 Apr 11, 2024

kenodegard Apr 11, 2024

schuylermartin45 Apr 11, 2024

schuylermartin45 Apr 11, 2024 •

edited

Loading

		return RecipeParser(recipe)


		def load_recipe_convert(file_name: str) -> RecipeParserConvert:

		return f.read()


		def load_recipe(file_name: str) -> RecipeParser:

Refactors recipe conversion work #22

Refactors recipe conversion work #22

Conversation

schuylermartin45 commented Apr 8, 2024 • edited Loading

jezdez left a comment

Choose a reason for hiding this comment

jezdez commented Apr 8, 2024

schuylermartin45 commented Apr 8, 2024 • edited Loading

jezdez left a comment

Choose a reason for hiding this comment

schuylermartin45 commented Apr 8, 2024

marcelotrevisani commented Apr 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schuylermartin45 commented Apr 8, 2024 • edited Loading

schuylermartin45 commented Apr 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schuylermartin45 Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

schuylermartin45 commented Apr 8, 2024 •

edited

Loading

schuylermartin45 commented Apr 8, 2024 •

edited

Loading

schuylermartin45 commented Apr 8, 2024 •

edited

Loading

schuylermartin45 Apr 11, 2024 •

edited

Loading