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

Allow to set parameteres like threshold #102

Open
manycoding opened this issue Jun 5, 2019 · 5 comments
Open

Allow to set parameteres like threshold #102

manycoding opened this issue Jun 5, 2019 · 5 comments
Labels
good first issue Good for newcomers Type: Feature New feature or request

Comments

@manycoding
Copy link
Contributor

manycoding commented Jun 5, 2019

A reloadable config or passing arguments is needed, so anybody can set report_all() at once:
E.g. we have threshold for coverage diff, which defaults to 0.2, so the config might look like:

config = Config()
config.threshold = 0.1
Arche(source, target, config).report_all()
@manycoding manycoding added Type: Feature New feature or request good first issue Good for newcomers labels Jun 5, 2019
@manycoding
Copy link
Contributor Author

It can be:

  1. A config
  2. Parameters from top-level - arche.report_all(threshold=0.2)
  3. Environment variables - in a notebook
%env THRESHOLD=0.2
...
arche.report_all()

@manycoding
Copy link
Contributor Author

@ejulio Any comments or ideas?

@manycoding manycoding changed the title config with default const Allowing set parameteres like threshold Jun 6, 2019
@manycoding manycoding changed the title Allowing set parameteres like threshold Allow to set parameteres like threshold Jun 6, 2019
@ejulio
Copy link

ejulio commented Jun 17, 2019

@manycoding , my idea here is, if it is only a config for report_all, then it would be better to set it only in report_all.

Since most of the time arche runs in jupyter notebooks, I don't think an environment variable is a good choice, at least at first.

So, my idea of an API would be

a = Arche(source, target)
a.report_all(
threshold=0.2
)
# or
a.report_all({
"threshold": 0.2
})

Both are fine and can be accomplished by **kwargs or **config (to use a better name)

@manycoding
Copy link
Contributor Author

@ejulio Do you think these threshold should be treated equally or be separated?

  1. Coverage diff between jobs (currently 0.1 for errors and 0.05 for warnings)
  2. Category diff between jobs (0.2, 0.1)
  3. Boolean diff between jobs (0.1, 0.05)

@ejulio
Copy link

ejulio commented Jun 18, 2019

IMHO, they should be different.
I might accept a diff of 0.3 in categories but only 0.05 for coverage for example.
So

a = Arche(source, target)
a.report_all(
    coverage_threshold=(0.1, 0.05),
    category_threshold=(0.3, 0.1)
)

Though I'm not sure about tuples in the example above.
I don't know what the numbers mean and their use cases unless I read the docs/underlying code.

Maybe NamedTuples or a simple data class.

@dataclass
class ReportThreshold:
    error_threshold: float
    warning_threshold: float

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers Type: Feature New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants