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

Refactoring for subset compile #549

Closed
wants to merge 2 commits into from
Closed

Refactoring for subset compile #549

wants to merge 2 commits into from

Conversation

dstadelm
Copy link
Contributor

This refactores the functions get_files_in_compile_order and get_dependencies_in_compile_order in the first commit. This was done as quite a lot of code is duplicate code.
In the second commit the SourceFile, VHDLSourceFile and VerilogSourceFile classes are refactored to a seperate module. As pycodestyle complained that the file project.py is too long.

David Stadelmann added 2 commits September 23, 2019 16:32
…_order

These two functions used a lot of similar code. The code is now split
into functions. These will also be used in future for the functionality
to compile only the required subset of files.
SourceFile, VerilogSourceFile and VHDLSourceFile now reside in own
module source_file
@eine
Copy link
Collaborator

eine commented Sep 24, 2019

Hi @dstadelm! It seems that this PR is a subset of #550. Could you, please, make it explicit? I'd suggest adding Ref #549 or Ref #550 in both comments, and adding some sentence in #550 explaining that it should not be reviewed until this is merged. Alternatively, you can edit the title to number ([1/2], [2/2], etc.) a series of related PRs.

Regarding this PR, would you mind either elaborating your first comment or further splitting these commits? The main motivation seems to be to reduce duplicate code. However, the proposed implementation is 10% larger. It might be legitimate, but it is hard to see it in the current state. For example, it seems that 5 lines of code are split to a new _get_affected_files function. Should that be a commit on it's own, it'd be easier to see where is it called from and how it helps with reducing code duplication.

"""
Get a list of all files in compile order
incremental -- Only return files that need recompile if True
:param incremental: -- Only return files that need recompile if True
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is -- a typo?

Also, please, add descriptions for dependency_graph and files too.

@dstadelm
Copy link
Contributor Author

Shure thing. I'll create a new pull request.

Regards
Dave

@dstadelm dstadelm closed this Sep 24, 2019
@eine
Copy link
Collaborator

eine commented Sep 24, 2019

Shure thing. I'll create a new pull request.

It is up to you, of couse; but it is not required to close and open new PRs for these changes. You can just push commits or force-push this branch. The advantage of doing so is that it is easier for us to see previous comments/discussions. It is useful to see different versions of a contribution, because it helps understand which decisions were made.

@dstadelm
Copy link
Contributor Author

Ok, I didn't know about that, especially the force push.

My new pull request does not comply with one pylint instance check. I don't know why, because it is complaining about code I didn't touch....

This pull request was closed.
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.

2 participants