Skip to content

Refactor sub scvar#48

Merged
andreab1997 merged 9 commits into
mainfrom
46-refactor-sub_scvar
Oct 13, 2022
Merged

Refactor sub scvar#48
andreab1997 merged 9 commits into
mainfrom
46-refactor-sub_scvar

Conversation

@andreab1997
Copy link
Copy Markdown
Contributor

No description provided.

@andreab1997 andreab1997 changed the base branch from testing to main October 5, 2022 16:07
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 5, 2022

Codecov Report

Merging #48 (a5358a1) into main (d67e52c) will increase coverage by 0.59%.
The diff coverage is 90.90%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #48      +/-   ##
==========================================
+ Coverage   88.35%   88.94%   +0.59%     
==========================================
  Files          16       16              
  Lines         601      606       +5     
==========================================
+ Hits          531      539       +8     
+ Misses         70       67       -3     
Flag Coverage Δ
bench 87.41% <90.90%> (+0.60%) ⬆️
unittests 47.35% <18.18%> (+0.27%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/pineko/cli/check.py 96.07% <90.90%> (+6.94%) ⬆️

@andreab1997 andreab1997 linked an issue Oct 5, 2022 that may be closed by this pull request
Comment thread src/pineko/cli/check.py Outdated
@alecandido
Copy link
Copy Markdown
Collaborator

I would propose to use enum for them, something like:

from enum import Enum, auto

class Scale(Enum):
    REN = auto()
    FACT = auto()

@click.argument(
    "scale",
    metavar="SCALE",
    type=click.Choice(list(map(lambda x: x.name.lower(), Scale)), case_sensitive=False),
)
...

It is more explicit, and may give more introspection in debugging.

@andreab1997
Copy link
Copy Markdown
Contributor Author

I would propose to use enum for them, something like:

from enum import Enum, auto

class Scale(Enum):
    REN = auto()
    FACT = auto()

@click.argument(
    "scale",
    metavar="SCALE",
    type=click.Choice(list(map(lambda x: x.name.lower(), Scale)), case_sensitive=False),
)
...

It is more explicit, and may give more introspection in debugging.

Do you mean something similar to what I have done in the last commit?

Comment thread src/pineko/cli/check.py Outdated
Comment thread src/pineko/cli/check.py Outdated
Comment thread src/pineko/cli/check.py Outdated
Comment thread src/pineko/cli/check.py Outdated
@alecandido
Copy link
Copy Markdown
Collaborator

Do you mean something similar to what I have done in the last commit?

Similar yes, but we can still improve: I added a bunch of comments.

I trust that is working, but since our code is growing, I'm trying to focus on improving the quality, to make it more maintainable (just because we're scaling, and trying to stabilize our infrastructure).

andreab1997 and others added 2 commits October 12, 2022 10:54
Co-authored-by: Alessandro Candido <candido.ale@gmail.com>
Comment thread src/pineko/cli/check.py Outdated
Comment thread src/pineko/cli/check.py Outdated
andreab1997 and others added 2 commits October 12, 2022 12:06
Co-authored-by: Alessandro Candido <candido.ale@gmail.com>
Co-authored-by: Alessandro Candido <candido.ale@gmail.com>
Comment thread src/pineko/cli/check.py Outdated
Comment thread src/pineko/cli/check.py Outdated
Comment thread src/pineko/cli/check.py Outdated
Comment thread src/pineko/cli/check.py Outdated
Comment thread src/pineko/cli/check.py Outdated
@andreab1997
Copy link
Copy Markdown
Contributor Author

Ok thank you @alecandido for the suggestions, this should be fine now

@andreab1997
Copy link
Copy Markdown
Contributor Author

I think we can merge. Thanks for this!

@andreab1997 andreab1997 merged commit 1b222e1 into main Oct 13, 2022
@felixhekhorn felixhekhorn deleted the 46-refactor-sub_scvar branch October 13, 2022 11:34
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.

Refactor sub_scvar

4 participants