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 model names to contain periods #121

Closed

Conversation

agile
Copy link
Contributor

@agile agile commented Nov 12, 2019

We are probably outliers here, but our models often have periods in their names which is not handled very well by the model/explore filtering.

Alternatively, I wonder if we could we use another delimiter than period, such as a slash '/' instead?

@agile agile force-pushed the handle_periods_in_model_names branch from e0a88cc to e8730f1 Compare November 12, 2019 16:09
@codecov
Copy link

codecov bot commented Nov 12, 2019

Codecov Report

Merging #121 into master will increase coverage by 0.87%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
+ Coverage   65.16%   66.04%   +0.87%     
==========================================
  Files           9        9              
  Lines         709      692      -17     
==========================================
- Hits          462      457       -5     
+ Misses        247      235      -12
Impacted Files Coverage Δ
spectacles/validators.py 59.65% <100%> (+2.66%) ⬆️

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 699ea9a...e8730f1. Read the comment docs.

@DylanBaker
Copy link
Collaborator

@agile I somehow didn't realise you could have periods in a model name. Do you know if it is possible to have them in explore names?

I am inclined to think that you're correct about this being an outlier instance, but we should solve it nonetheless. I do think it would be easiest to take the alternative approach and just delimit on something that can't be in model or explore names. @joshtemple Do you have a strong preference?

@iserko
Copy link
Contributor

iserko commented Nov 13, 2019

Why not just separate it into two CLI parameters.

# current (specific explore)
--explores mymodel.myexplore
# would be replaced by
--model mymodel --explore myexplore
# current (whole model)
--explores mymodel.*
# would be replaced by
--model mymodel
# current (a specific explore in all models)
# this shouldn't be possible now anyway, but not sure if the code catches it
# or maybe it is something people actually need to run against?
--explores *.myexplore
# should definitely fail in the new parser if you don't specify --model as well
--explore myexplore

@DylanBaker
Copy link
Collaborator

How would you want it to work if you were interested in testing explores across multiple models.

i.e. in the current setup:

--explores model_one.explore_one model_two.explore_two

@DylanBaker
Copy link
Collaborator

@agile It looks like a slash / should work. It isn't allowed in either a model or an explore name, which solved this problem. That would be our preferred solution I think.

@agile agile closed this Nov 15, 2019
@agile agile deleted the handle_periods_in_model_names branch November 15, 2019 04:35
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.

3 participants