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 makefile clean and phony targets #62

Merged
merged 4 commits into from
Dec 4, 2019
Merged

Conversation

austinabell
Copy link
Contributor

Sorry I forgot to comment on the PR that the encoding package was missing, so I added it and also included phony target definitions (makefile uses file system over definitions in file, so defines that the commands are referencing the other targets and not a file target) and a test command while I was here

Copy link
Member

@GregTheGreek GregTheGreek left a comment

Choose a reason for hiding this comment

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

LGTM

side note:
I dont ever see us having clean, clean-all or lint dir in our root, but probably safer than sorry

@austinabell
Copy link
Contributor Author

austinabell commented Dec 4, 2019

LGTM

side note:
I dont ever see us having clean, clean-all or lint dir in our root, but probably safer than sorry

well, you would have to clean and recompile to check for linting issues, since warnings will only show when it is compiled. So if something compiles with a warning and that part of the code is not changed, it will not show the warning again until cleaning the built packages from the codebase. This is why it is cleaned before linting.

Wouldn't make sense to build or run inside each directory individually since it would create copies of built code from the crate and dependencies in each directory. This is the benefit to being able to run everything from root

@austinabell
Copy link
Contributor Author

added other missing packages since review @GregTheGreek @dutterbutter

@GregTheGreek
Copy link
Member

LGTM
side note:
I dont ever see us having clean, clean-all or lint dir in our root, but probably safer than sorry

well, you would have to clean and recompile to check for linting issues, since warnings will only show when it is compiled. So if something compiles with a warning and that part of the code is not changed, it will not show the warning again until cleaning the built packages from the codebase. This is why it is cleaned before linting.

Wouldn't make sense to build or run inside each directory individually since it would create copies of built code from the crate and dependencies in each directory. This is the benefit to being able to run everything from root

Sorry, I was referencing including the above make commands in the .PHONE

@austinabell austinabell merged commit 3438654 into master Dec 4, 2019
@austinabell austinabell deleted the austin/makefilefix branch December 4, 2019 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants