Skip to content
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

👌 Exclude external needs from needs_id_regex check #1108

Conversation

David-Le-Nir
Copy link
Contributor

fix for #1099

@David-Le-Nir David-Le-Nir force-pushed the fix/issue_1099_needs_id_regex_on_external_needs branch from f18f28c to 4a818e9 Compare February 15, 2024 09:16
Copy link

codecov bot commented Feb 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.90%. Comparing base (cf5598f) to head (8b355fb).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1108   +/-   ##
=======================================
  Coverage   85.90%   85.90%           
=======================================
  Files          56       56           
  Lines        6511     6511           
=======================================
  Hits         5593     5593           
  Misses        918      918           
Flag Coverage Δ
pytests 85.90% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@danwos danwos left a comment

Choose a reason for hiding this comment

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

Thanks for the quick PR 👍

The change in need.py looks good, as well as the updated test case.

However, I think we need to clean this PR a little bit. :)

Version changes are introduced by a separate PR. Makes it easier to rewind single commits. So please remove this.

.DS_Store and sphinxcontrib-needs.iml are also not needed.
It's okay to add them to .gitignore if it simplifies your life.

And it would be also good to add a small line to docs/changelog.rst for a 2.1.0 release.

@danwos
Copy link
Member

danwos commented Feb 15, 2024

Ohh and please ignore the failing pre-commit action.
Looks like a new mypy version finds much more now. I deal with this in a different PR and will merge this PR by hand.

@David-Le-Nir
Copy link
Contributor Author

Version changes are introduced by a separate PR. Makes it easier to rewind single commits. So please remove this.

.DS_Store and sphinxcontrib-needs.iml are also not needed.
It's okay to add them to .gitignore if it simplifies your life.

And it would be also good to add a small line to docs/changelog.rst for a 2.1.0 release.

yes, sorry, my CLI shortcuts are a bit too "shortcutting" ... I'll fix that
yes, for the changelog, I didn't know which version to set. Also should I update the pyproject.toml to set 2.1.0 for version ?

@David-Le-Nir David-Le-Nir force-pushed the fix/issue_1099_needs_id_regex_on_external_needs branch 2 times, most recently from e779a8f to 0a1c6c8 Compare February 15, 2024 10:01
@danwos
Copy link
Member

danwos commented Feb 15, 2024

No, we need to raise or better add the version just in the changelog.
All the other locations (pyproject.toml, needs.py, ...) will be done by a separate PR.

@David-Le-Nir David-Le-Nir force-pushed the fix/issue_1099_needs_id_regex_on_external_needs branch from 0a1c6c8 to 00d5aba Compare February 15, 2024 10:11
@danwos
Copy link
Member

danwos commented Feb 15, 2024

I was asked by @chrisjsewell to hold this PR back for some time.
He may have some concerns but is sitting on a train right now.

Copy link
Member

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

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

This is all good thanks, if you can apply the changes and fix the merge conflicts

docs/changelog.rst Outdated Show resolved Hide resolved
sphinx_needs/api/need.py Outdated Show resolved Hide resolved
@chrisjsewell chrisjsewell changed the title exclude external needs from the identifier regex check (useblocks#1099) 👌 Exclude external needs from needs_id_regex check Feb 26, 2024
@chrisjsewell chrisjsewell linked an issue Feb 26, 2024 that may be closed by this pull request
@David-Le-Nir David-Le-Nir force-pushed the fix/issue_1099_needs_id_regex_on_external_needs branch from be3f6fe to 8b355fb Compare February 26, 2024 08:07
Copy link
Member

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

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

cheers

@chrisjsewell chrisjsewell requested a review from danwos February 26, 2024 08:10
@danwos danwos merged commit a037336 into useblocks:master Feb 26, 2024
19 checks passed
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.

needs_id_regex should not apply on needs_external_needs
3 participants