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

Fix attribute exclusion #884

Merged
merged 4 commits into from
Jun 26, 2020
Merged

Fix attribute exclusion #884

merged 4 commits into from
Jun 26, 2020

Conversation

bddckr
Copy link
Contributor

@bddckr bddckr commented Jun 23, 2020

Closes #883

The following changes have been added additionally:

  • Reduce allocations to be done once only.
  • Update the documentation to get rid of a mistake: Fully qualified attribute type names are actually not supported. I checked all the other documentation documents - they are already OK.

@dnfadmin
Copy link

dnfadmin commented Jun 23, 2020

CLA assistant check
All CLA requirements met.

@MarcoRossignoli
Copy link
Collaborator

@MarcoRossignoli MarcoRossignoli added the bug Something isn't working label Jun 24, 2020
@bddckr
Copy link
Contributor Author

bddckr commented Jun 25, 2020

@MarcoRossignoli I totally forgot about those 😅 Added now. Let me know if you think the test changes are OK. I figured there's no need to create a custom attribute type as the one that originally gave us issues can be referenced directly in the tests. I also didn't add any specific test for this and instead updated the various exclusion tests so they all test this case, too.

@bddckr
Copy link
Contributor Author

bddckr commented Jun 25, 2020

I had to update the CecilSymbolHelperTests to make the tests pass. I think that is needed due to me adding a test class into DeclaredConstructorClass, but let me know if that is wrong.

@MarcoRossignoli
Copy link
Collaborator

Thanks I'll take a look asap

@MarcoRossignoli
Copy link
Collaborator

MarcoRossignoli commented Jun 26, 2020

I had to update the CecilSymbolHelperTests to make the tests pass. I think that is needed due to me adding a test class into DeclaredConstructorClass, but let me know if that is wrong.

It's ok, it's related to new using on top of sample file, moved all lines below

Copy link
Collaborator

@MarcoRossignoli MarcoRossignoli left a comment

Choose a reason for hiding this comment

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

LGTM thans's!

@MarcoRossignoli MarcoRossignoli merged commit f760787 into coverlet-coverage:master Jun 26, 2020
@bddckr bddckr deleted the fix/attribute-exclusion branch June 26, 2020 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Attribute exclusion does not work if attribute name does not end with "Attribute"
3 participants