Skip to content

Conversation

@horw
Copy link
Collaborator

@horw horw commented Jan 3, 2025

Description

Reorganized the constants and soc_header imports in bool_parser, moving them closer to their usage.

Related

Testing


Checklist

Before submitting a Pull Request, please ensure the following:

  • 🚨 This PR does not introduce breaking changes.
  • All CI checks (GH Actions) pass.
  • Documentation is updated as needed.
  • Tests are updated or added as necessary.
  • Code is well-commented, especially in complex areas.
  • Git history is clean — commits are squashed to the minimum necessary.

@horw horw requested a review from hfudev January 3, 2025 10:27
@github-actions
Copy link

github-actions bot commented Jan 3, 2025

Coverage

Coverage Report
FileStmtsMissCoverMissing
esp_bool_parser
   bool_parser.py1233175%49, 73–107, 128, 131, 158–165, 170, 178, 182
   constants.py411368%19–20, 45–57
   soc_header.py684140%54–64, 77–82, 86–93, 97–138
   utils.py11373%21, 25–26
TOTAL2468864% 

Tests Skipped Failures Errors Time
49 0 💤 0 ❌ 0 🔥 0.638s ⏱️

Copy link
Member

@hfudev hfudev left a comment

Choose a reason for hiding this comment

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

LGTM.


if self.attr == 'CONFIG_NAME':
return config_name
from .soc_header import SOC_HEADERS
Copy link
Member

Choose a reason for hiding this comment

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

How about we prioritize loading the environment variables? so if I rely on one env var defined locally, there's no need to check and load the IDF_PATH (which may not exist)

The original comment:

for non-keyword cap words, check if it is defined in the environment variables

no idea why I put it here... I can't think of a real risk about it.

Copy link
Member

Choose a reason for hiding this comment

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

overall the sequence could be

  • additional_attr
  • IDF_TARGET
  • CONFIG_NAME
  • env var
  • IDF_VERSION(_XXX) - lazy-load
  • SOC_CAPS - lazy-load

@hfudev
Copy link
Member

hfudev commented Jan 3, 2025

after this got merged, shall we release 0.1.0 and add this dependency in idf-build-apps and pytest-embedded?

@horw
Copy link
Collaborator Author

horw commented Jan 3, 2025

after this got merged, shall we release 0.1.0 and add this dependency in idf-build-apps and pytest-embedded?

Sure, let's do it. I will start with pytest-embedded.

@horw horw merged commit 38cb76b into main Jan 3, 2025
4 checks passed
@hfudev hfudev deleted the lazy-load branch January 3, 2025 13:09
@github-actions github-actions bot changed the title feat: lazy loading for constants and soc_header in bool_parser feat: lazy loading for constants and soc_header in bool_parser (RDT-1071) Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants