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

Adding Support for filtering by tag with variables. #457

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

inquisitev
Copy link
Contributor

@inquisitev inquisitev commented Oct 2, 2023

This will require an update to tag-expressions

requires: timofurrer/tag-expressions#2 to avoid parsing exception

as for the change log, i see an unreleased section, so i am assuming that not every change gets a release. is this how it works in practice? should this get a new version? and if the unreleased section is never moved, may i remove it to reduce confusion?

closes #453

@fliiiix
Copy link
Member

fliiiix commented Oct 3, 2023

nice i try to release this somewhere this week
i need to figure out why the github actions do not run and review the change

And for releasing you could have added your feature just to the unreleased section and then at somepoint i would create a new release that would be the general idea but since you also update the version already we can also take that 👍

@fliiiix fliiiix self-requested a review October 3, 2023 07:16
Copy link
Member

@fliiiix fliiiix left a comment

Choose a reason for hiding this comment

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

i guess this change should also be reflected in the documentation somewhere?

radish/parser.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (d08899b) 87.26% compared to head (af6b8ec) 87.26%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #457   +/-   ##
=======================================
  Coverage   87.26%   87.26%           
=======================================
  Files          39       39           
  Lines        2380     2380           
=======================================
  Hits         2077     2077           
  Misses        303      303           
Flag Coverage Δ
unittests 87.26% <100.00%> (ø)

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

Files Coverage Δ
radish/hookregistry.py 100.00% <100.00%> (ø)
radish/parser.py 99.14% <100.00%> (ø)

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

Copy link
Member

@fliiiix fliiiix left a comment

Choose a reason for hiding this comment

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

lgtm so there is still the question if we can filter for @tag value somehow otherwise i think that should be more or less ready to be merged

docs/tutorial.rst Show resolved Hide resolved
@fliiiix
Copy link
Member

fliiiix commented Oct 5, 2023

I try to play around with it a bit so i can merge it on the weekend

@inquisitev
Copy link
Contributor Author

lgtm so there is still the question if we can filter for @tag value somehow otherwise i think that should be more or less ready to be merged

It would likely require replacing all white space with some other character before replacing the parens in tag expressions.

@fliiiix
Copy link
Member

fliiiix commented Oct 8, 2023

@inquisitev please take a look i update the testing a bit and extend the docs and took the liberty to restructure that as a single commit without the release since that should happen from main branch

Let me know if i missed anything and if the updated docs make sense to you.

Minor side-note:

  • is there a reason that you used merges instead of rebases?
  • I think your name contains a extra space character between firstname and lastname Trevor Keegan
    image
    I assume thats not on purpose so you might take a look at your git config

@inquisitev
Copy link
Contributor Author

@inquisitev please take a look i update the testing a bit and extend the docs and took the liberty to restructure that as a single commit without the release since that should happen from main branch

Let me know if i missed anything and if the updated docs make sense to you.

Minor side-note:

  • is there a reason that you used merges instead of rebases?
  • I think your name contains a extra space character between firstname and lastname Trevor Keegan
    image
    I assume thats not on purpose so you might take a look at your git config

Looks good to me.

"is there a reason that you used merges instead of rebases?" - I'm not sure why its using merge, i think i squashed earlier (if i did i used git rebase -i HEAD~n). manually squashing is new to me as we just let github squash on pr merge at work.

"I think your name contains a extra space ..." - thanks for catching that, ive updated my config. whoops

@fliiiix fliiiix merged commit a515e15 into radish-bdd:main Oct 11, 2023
11 checks passed
@inquisitev inquisitev deleted the filter-by-tag-with-value branch October 11, 2023 22:18
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.

Tags do not contain value
2 participants