Skip to content

Tried out black on the project. - #18

Open
scolby33 wants to merge 3 commits into
developfrom
blackened
Open

Tried out black on the project.#18
scolby33 wants to merge 3 commits into
developfrom
blackened

Conversation

@scolby33

@scolby33 scolby33 commented Jul 2, 2018

Copy link
Copy Markdown
Owner

What do you think?

@scolby33
scolby33 requested a review from cthoyt July 2, 2018 22:55
scolby33 added 2 commits July 2, 2018 16:00
It interferes with black and is apparently not supported by the PEP anyway.
@codecov

codecov Bot commented Jul 2, 2018

Copy link
Copy Markdown

Codecov Report

Merging #18 into develop will not change coverage.
The diff coverage is 49.39%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop      #18   +/-   ##
========================================
  Coverage    41.02%   41.02%           
========================================
  Files           20       20           
  Lines          763      763           
  Branches        71       71           
========================================
  Hits           313      313           
  Misses         436      436           
  Partials        14       14
Impacted Files Coverage Δ
src/ocspdash/__init__.py 100% <ø> (ø) ⬆️
src/ocspdash/web/blueprints/api.py 0% <0%> (ø) ⬆️
src/ocspdash/cli.py 0% <0%> (ø) ⬆️
src/ocspdash/web/admin.py 0% <0%> (ø) ⬆️
src/ocspdash/ocspscrape.py 0% <0%> (ø) ⬆️
src/ocspdash/web/app.py 0% <0%> (ø) ⬆️
src/ocspdash/web/extension.py 0% <0%> (ø) ⬆️
src/ocspdash/web/proxies.py 0% <0%> (ø) ⬆️
src/ocspdash/web/__init__.py 0% <0%> (ø) ⬆️
src/ocspdash/web/blueprints/__init__.py 0% <0%> (ø) ⬆️
... and 7 more

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 1d36b08...3735886. Read the comment docs.

Comment thread src/ocspdash/cli.py

invite_id, invite_validator = m.create_location(location_name)

click.echo(base64.urlsafe_b64encode(b''.join((invite_id, invite_validator))).decode("utf-8"))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this line is too long (and has too many inner calls) so you should change it to have more temporary variables

Comment thread src/ocspdash/constants.py

#: The directory in which data for OCSP Dashboard is stored. Can be set from the environment variable
#: ``OCSPDASH_DIRECTORY`` or defaults to ``~/.ocspdash``
OCSPDASH_DIRECTORY = os.environ.get('OCSPDASH_DIRECTORY', os.path.join(os.path.expanduser('~'), '.ocspdash'))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

should save default OCSPdash location as a variable as well

Comment thread src/ocspdash/constants.py
if not os.path.exists(OCSPDASH_DIRECTORY):
os.makedirs(OCSPDASH_DIRECTORY)

OCSPDASH_DEFAULT_CONNECTION = 'sqlite:///' + os.path.join(OCSPDASH_DIRECTORY, 'ocspdash.db')

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't like what it did here

Comment thread src/ocspdash/constants.py

#: The rate limit for connecting to Censys. Can be set from the environmental variable ``OCSPDASH_RATE`` or defaults
# to ``0.2``.
CENSYS_RATE_LIMIT = float(os.environ.get('OCSPDASH_RATE', 0.2)) # max requests per second

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

too bad os.environ.get doesn't work like the one from flask.request.args.get

Comment thread src/ocspdash/constants.py
) # max requests per second

OCSPDASH_USER_AGENT_IDENTIFIER = f'OCSPdash/{VERSION}'
OCSPDASH_USER_AGENT = ' '.join([requests.utils.default_user_agent(), OCSPDASH_USER_AGENT_IDENTIFIER])

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why do you write it with a join on a space

Comment thread src/ocspdash/models.py
return f'Invite for {self.name}'

def to_json(
self): # TODO: this gets returned by the /register endpoint and some of this info could be considered private. Should it be removed from the to_json?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this is not good. move this TODO

Comment thread src/ocspdash/models.py
'validator_hash': self.validator_hash,
'pubkey': str(self.pubkey),
'key_id': str(self.key_id),
'results': [

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

agreed

Comment thread src/ocspdash/models.py
"""Return a representation of the instance suitable for passing in to JSON conversion."""
return {
'id': self.id,
'location': {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

disagree

print(f'public key:\t{claims["pk"]}'.expandtabs(7)[:78] + '..')
print(f'invite token:\t{claims["token"]}'.expandtabs(7))
else:
token = scrape(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

... fine

format=serialization.PublicFormat.SubjectPublicKeyInfo,
)
).decode('utf-8')
public_key = b64encode(private_key.public_key().public_bytes(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

also agreed. this is TOO complicated

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