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

Basic project infrastructure #1

Merged
merged 5 commits into from
Mar 24, 2021

Conversation

cmatsuoka
Copy link
Collaborator

Add the initial project infrastructure including: main Makefile, GitHub
test workflows, basic documentation files, tool configuration files,
and setup dependencies.

Signed-off-by: Claudio Matsuoka [email protected]

Add the initial project infrastructure including: main Makefile, GitHub
test workflows, basic documentation files, tool configuration files,
and setup dependencies.

Signed-off-by: Claudio Matsuoka <[email protected]>
@cmatsuoka cmatsuoka requested a review from cjp256 March 23, 2021 20:49
Copy link
Contributor

@cjp256 cjp256 left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a few nits/suggestions.

@@ -0,0 +1,19 @@
# -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*-
#
# Copyright (C) 2021 Canonical Ltd
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: on a previous PR, we decided to use Copyright 2021 Canonical Ltd.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The license text uses both the work and the symbol, however the copyright office documentation examples use the form you mentioned so I'll change the notices to use it.

pyproject.toml Outdated
max-attributes = 15
max-args= 6
max-locals = 16
good-names = "f,a,e,lf,id,do_GET"
Copy link
Contributor

Choose a reason for hiding this comment

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

interesting option...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. f is a canonical variable for files and craft-parts uses lf for its LifecycleManager. id and do_GET are required by external components, but a and e can be removed. None of these are required at this point, so I'll remove them and add as needed in each PR.


from pathlib import Path

TESTS_DIR = Path(__file__).parent
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently nothing. It will be used later to locate resources inside the testing tree, but it can be removed from this PR.

Makefile Outdated
@@ -0,0 +1,143 @@
.DEFAULT_GOAL := help
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth resyncing against craft-providers Makefile to address all the feedback I got on this one :D

setup.cfg Outdated
python_version = 3.8

[pycodestyle]
ignore = E402, E501, W503
Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw, in craft-providers I've been trying to maintain the list of explanations alongside the codes. e.g.

[pydocstyle]
# D105 Missing docstring in magic method (reason: magic methods already have definitions)
# D107 Missing docstring in __init__ (reason: documented in class docstring)
# D203 1 blank line required before class docstring (reason: pep257 default)
# D213 Multi-line docstring summary should start at the second line (reason: pep257 default)
# D215 Section underline is over-indented (reason: pep257 default)
ignore = D105, D107, D203, D213, D215

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Resynced setup.cfg against craft-providers as well.

Signed-off-by: Claudio Matsuoka <[email protected]>
Signed-off-by: Claudio Matsuoka <[email protected]>
The standard copyright notice adopted in craft projects uses only the
copyright word instead of word and symbol. This matches the example
notice given in https://www.copyright.gov/comp3/chap2200/ch2200-notice.pdf

Signed-off-by: Claudio Matsuoka <[email protected]>
@cmatsuoka cmatsuoka force-pushed the import/project-infrastructure branch from 151cfdb to 01b88bc Compare March 24, 2021 13:55
@@ -0,0 +1,88 @@
# Copyright 2021 Canonical Ltd
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Copyright 2021 Canonical Ltd
# Copyright 2021 Canonical Ltd.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will normalize these for all files in the next PR.

@@ -0,0 +1,68 @@
#!/usr/bin/env python
#
# Copyright 2021 Canonical Ltd
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Copyright 2021 Canonical Ltd
# Copyright 2021 Canonical Ltd.

Copy link
Contributor

@cjp256 cjp256 left a comment

Choose a reason for hiding this comment

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

LGTM

@cmatsuoka cmatsuoka merged commit 50c7145 into canonical:main Mar 24, 2021
tigarmo referenced this pull request in tigarmo/craft-parts Sep 15, 2022
http_client: introduce new module and related error class (CRAFT-443)
lengau pushed a commit that referenced this pull request Oct 30, 2023
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