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

Initial Version #1

Merged
merged 100 commits into from
Mar 13, 2018
Merged

Initial Version #1

merged 100 commits into from
Mar 13, 2018

Conversation

jtdoepke
Copy link
Contributor

@jtdoepke jtdoepke commented Mar 6, 2018

This PR is the ongoing work to populate an initial version of this library.

TODO

Boilerplate

  • CONTRIBUTING.rst

Docs

  • Finish documenting everything.

Testing

Review

  • Get this reviewed (preferably an email to all-IT).

Deployment

  • Figure out how to have a PyPI project for an organization (Mintel) or multiple owners. Populate this TODO section. (How does Django/other big projects do it?)

* Added the basic Python package stuff (setup.py, etc).
* Added a dummy test to setup Travis-CI.
reStructuredText is the prefered format of PyPI.
We don't need coverage of the `_version.py` file.
We have Travis-CI. Why even bother with Tox?
So we fail faster.
Until pypa/pipenv#857 is fixed,
pipenv and hypothesis don't quite play will together.
From the hypothesis [docs](https://hypothesis.readthedocs.io/en/latest/supported.html#testing-frameworks):

> However you should probably not use Coverage, Hypothesis and PyPy together.
> Because Hypothesis does quite a lot of CPU heavy work compared to normal tests,
> it really exacerbates the performance problems the two normally have working together.
Hypothesis tests just don't seem to work well in pypy.
Seem to be running into [this](pypa/setuptools#951)
with Python 3.4 tests.
@@ -0,0 +1,24 @@
[[source]]

url = "https://pypi.python.org/simple"
Copy link

Choose a reason for hiding this comment

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

Rather trivial, but this URL redirects to add a tailing / . This makes me wonder how much energy and time amending this would save v.s. how much it takes to report+act on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depends on what pipenv's doing with it. I bet it's just urljoin()ing it with each package name.

for line in logs_generator:
if self.encoding is not None and isinstance(line, bytes):
line = line.decode(self.encoding)
self.logger.log(self.log_level, line.rstrip())
Copy link

Choose a reason for hiding this comment

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

pedantic: I am wondering if line.rstrip() is reasonable to relax to line or line[:-len(os.linesep)]. I am happy with it as it is anyway.

Copy link

Choose a reason for hiding this comment

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

thinking of it, it may be good in some imaginary scenario to give additional context to the logger, e.g. extra=container. Then again using loggers like myapp.tests.test_name.container_name would probably be a better way for people to get that information if they really wanted it.

Copy link
Contributor Author

@jtdoepke jtdoepke Mar 7, 2018

Choose a reason for hiding this comment

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

Which line separator you see would depending on the container. Probably have to do

line = re.sub(r'(?:\r?\n|\n\r)$', '', line)
  • Remove only single newline from each line in DockerTailer

The extra context would be good to do later. Raise an issue!

Copy link

Choose a reason for hiding this comment

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

Good point, I wasn't thinking with portals. Err, containers.

@jtdoepke jtdoepke added this to the 0.1.0 milestone Mar 7, 2018
@jtdoepke
Copy link
Contributor Author

jtdoepke commented Mar 8, 2018

@trickv @nabadger I think we're going to need a Mintel user for https://pypi.python.org/pypi and https://testpypi.python.org/pypi to be one of the package owners and to let Travis-CI push package versions.

@jtdoepke
Copy link
Contributor Author

jtdoepke commented Mar 8, 2018

Since this is basically functional, I'm thinking of merge to master and handling to remaining TODOs as separate issues (pending review).

setup.cfg Outdated
# H401: docstring should not start with a space
# H403: multi line docstrings should end on a new line
# H404: multi line docstring should start without a leading new line
E133,D104,D105,D107,D203,D212,D213,D301,D400,D404,D401,D402,F812,H101,H202,H401,H403,H405

Choose a reason for hiding this comment

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

Are all of these ignore rules really required? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll check. It is partially cargo-culted. Recording why we're ignoring them would also be a good idea.

  • Review flake8 ignored rules

Makefile Outdated
PIPENV := PIPENV_VENV_IN_PROJECT=1 pipenv
WITH_PIPENV := $(PIPENV) run

help: ## print this help

Choose a reason for hiding this comment

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

I don't know much about makefiles, but don't you also need to mark help as being .PHONY? It is in the link below.

The cache is huge; it's probably not speeding anything up at this point.
All these tests are expensive. We should only run them for

- master
- releases
- pull requests

[skip ci]
@jtdoepke jtdoepke force-pushed the initial branch 2 times, most recently from 77a130d to a616f71 Compare March 12, 2018 21:40
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.

None yet

3 participants