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

Minor fixes #40

Merged
merged 5 commits into from
Dec 26, 2021
Merged

Minor fixes #40

merged 5 commits into from
Dec 26, 2021

Conversation

umarcor
Copy link
Member

@umarcor umarcor commented Dec 26, 2021

Bug Fixes

  • Fix linter warnings.
  • Update Codacy target URLs, from manual to gh.

@umarcor umarcor added Enhancement New feature or request Bug Something isn't working and removed Enhancement New feature or request labels Dec 26, 2021
Copy link
Member

@Paebbels Paebbels left a comment

Choose a reason for hiding this comment

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

Maybe we split these fixes from fixes proposed by codacy.


Maybe we should also limit the staticmethod rule in codacy...

@@ -149,7 +149,8 @@ def _ParseFile(self, fileNode, fileset):
else:
self._ParseDefaultFile(fileNode, filePath, fileset)

def _ParseVHDLFile(self, fileNode, path, fileset):
@staticmethod
Copy link
Member

Choose a reason for hiding this comment

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

While it's possible to change it to staticmethod I'm not a fan of it for this usecase.

With staticmethod these internal helper function get public on the class and can be called from outside standalone, which makes no sense and has other risks.

@@ -140,7 +140,8 @@ def test_Files(self):


class Validate(TestCase):
def test_Design(self):
@staticmethod
def test_Design():
Copy link
Member

Choose a reason for hiding this comment

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

I think it's not good to convert tests to staticmethods.

Have you checked of pytest is finding these tests?

@codecov-commenter
Copy link

Codecov Report

Merging #40 (cda002d) into dev (e778a70) will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev      #40      +/-   ##
==========================================
+ Coverage   81.31%   81.37%   +0.05%     
==========================================
  Files           9        9              
  Lines        1504     1503       -1     
  Branches      243      243              
==========================================
  Hits         1223     1223              
+ Misses        213      212       -1     
  Partials       68       68              
Flag Coverage Δ
unittests 81.37% <ø> (+0.05%) ⬆️

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

Impacted Files Coverage Δ
pyEDAA/ProjectModel/__init__.py 72.17% <ø> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e778a70...cda002d. Read the comment docs.

@umarcor
Copy link
Member Author

umarcor commented Dec 26, 2021

I moved commits related to staticmethod into #41. Now, this PR contains blank line and list issues only.

@Paebbels Paebbels merged commit 74650f6 into dev Dec 26, 2021
@umarcor umarcor deleted the umarcor/dev branch December 30, 2021 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Development

Successfully merging this pull request may close these issues.

3 participants