nedbat / scriv

Changelog management tool
https://scriv.readthedocs.io
Apache License 2.0
256 stars 28 forks source link

yaml literals #70

Closed fkuep closed 1 year ago

fkuep commented 1 year ago

69

nedbat commented 1 year ago

Thanks! The quality failure is because of missing blank lines:

--- src/scriv/literals.py   2022-11-28 11:30:03.217489 +0000
+++ src/scriv/literals.py   2022-11-28 11:31:41.120974 +0000
@@ -13,10 +13,11 @@

 try:
     import yaml
 except ImportError:  # pragma: no cover
     yaml = None  # type: ignore
+

 def find_literal(file_name: str, literal_name: str) -> Optional[str]:
     """
     Look inside a file for a literal value, and return it.

@@ -111,10 +112,11 @@

     if isinstance(current_object, str):
         return current_object
     return None

+
 def find_yaml_value(data: MutableMapping[str, Any], name: str) -> Optional[str]:
     """
     Use a period-separated name to traverse a dictionary.

     Only string values are supported.

I've improved the check on main so that it would be more evident in the future.

Can you write some tests? The toml support has ones you can use as a model. Also, a scriv entry? :)

fkuep commented 1 year ago

Thanks! The quality failure is because of missing blank lines:

One is gone with the superfluous function, one blank line inserted.

I've improved the check on main so that it would be more evident in the future.

Thanks for making those easier to understand. I gave it a try here with little experiance and a simple text editor, so the likelihood of triggering the quality bots is high.

Can you write some tests? The toml support has ones you can use as a model.

I feared You'd say that :smile: I will try right now.

Also, a scriv entry? :)

How could I forget?

fkuep commented 1 year ago

I must be missing something simple: https://github.com/fkuep/scriv/actions/runs/3573755274/jobs/6008187744#step:5:21

nedbat commented 1 year ago

You need to add types-yaml to requirements/quality.in, then run make upgrade

fkuep commented 1 year ago

You need to add types-yaml to requirements/quality.in, then run make upgrade Thank You for the hint Ned, if You don't me asking in baby-steps , now I have: https://github.com/fkuep/scriv/actions/runs/3576852541/jobs/6015152869#step:5:190
Is that because I was running make upgrade on python 3.9 ?

nedbat commented 1 year ago

It looks like you ran make upgrade on Python 3.10, and yes, that's why the installation is failing. You need to run it on the lowest supported version, which is 3.7. If you can't, I can make a PR into this one later.

fkuep commented 1 year ago

run it on the lowest supported version, which is 3.7. If you can't, I can make a PR into this one later.

I will try

fkuep commented 1 year ago

I will pick that up tomorrow (it was a long day) .
Is it ok that the matrix- tests are failing on my fork ? It would be great to know , if I could start trying to write a test tomorrow. Thank You very much for Your patience Ned.
Is it ok to ask You so much still ?

nedbat commented 1 year ago

Sorry about the tests on your fork. I've fixed that test on master. Get that code, and your tests should pass.

Ask me anything you need, I really appreciate you putting in the work.

fkuep commented 1 year ago

Good morning Ned,

tests on your fork. I've fixed that test on master. Get that code, and your tests should pass.

I have added https://github.com/nedbat/scriv/commit/5a9c27877ab9a4482484a0dc439952450f7bca96

The matrix test still don't run.
Do I understand it correctly in "linters etc" the tests/test_literals.py test is not run yet ?

fkuep commented 1 year ago

I can now fail on purpose:
https://github.com/fkuep/scriv/actions/runs/3584124068/jobs/6030399247#step:8:54

and also run successful tests:
https://github.com/fkuep/scriv/actions/runs/3584100481/jobs/6030339738#step:8:37

I tried (modeled after toml) testing for installation of extra yaml , but that I don't understand yet.

I will now have a look at tests/test_gitinfo.py

fkuep commented 1 year ago

I beleive Your code assumes a github repo should end in .git https://github.com/nedbat/scriv/blob/main/src/scriv/gitinfo.py#L90

While my url line is:

origin https://github.com/fkuep/scriv (fetch)

From the test You even say that, but then gitinfo.py does not reflect that. https://github.com/fkuep/scriv/actions/runs/3584281900/jobs/6030754148#step:8:97

did I miss a patch ?

nedbat commented 1 year ago

Your pull request is helping me find all sort of problems to fix! Merge master again to fix the test_gitinfo problem, and removing your debug prints will solve one of the others. I haven't looked at the yaml failure.

fkuep commented 1 year ago

Merge master again

on my way..

fkuep commented 1 year ago

YESS!!!

image

fkuep commented 1 year ago

I am off the keyboard for 15 mins. ... celebratory coffee

fkuep commented 1 year ago

This is the one , I still need help with:
https://github.com/fkuep/scriv/actions/runs/3585042596/jobs/6032505316#step:8:42

nedbat commented 1 year ago

Looks like it's just a mismatch in the case of "yaml" between the message and the regex matching the message:

    def test_find_yaml_literal_fail_if_unavailable(monkeypatch):
        monkeypatch.setattr(scriv.literals, "yaml", None)
>       with pytest.raises(Exception, match="Can't read .+ without YAML support"):
E       AssertionError: Regex pattern did not match.
E        Regex: "Can't read .+ without YAML support"
E        Input: "Can't read 'foo.yml' without yaml support. Install with [yaml] extra"
fkuep commented 1 year ago

mismatch in the case of "yaml"

Thanks! https://github.com/fkuep/scriv/actions/runs/3585258868/jobs/6033012026#step:8:37

fkuep commented 1 year ago

Ned, that might be the last thing I can think of: While I was hunting this "yaml support" issue, I put pyyaml in test.in

(scriv_venv) root@buster:~/scriv/scriv# grep -ni yaml requirements/*.in
requirements/quality.in:19:types-pyyaml
requirements/test.in:11:pyyaml
(scriv_venv) root@buster:~/scriv/scriv# 

Should I remove that or put in quality.in ?
Any cleanup ? Docs addition ok ?

nedbat commented 1 year ago

I think you were right to put pyyaml into test.in

I've fixed the badge creation on master, if you merge one more time, your Coverage check will pass

The docs look fine. I think I will take an editing pass over that whole section, since the first sentence mentions "variables", which is a holdover from the only-.py days.

I think this might be ready to go!

fkuep commented 1 year ago

I've fixed the badge creation on master, if you merge one more time, your Coverage check will pass running now.

I don't mind though.

I think this might be ready to go!

GREAT!

Mind You - I have written an eMail to You with three little beginner Questions I have.
Specificially coined in the context of scriv, so it does not get too abstract.
I think You would need less than 15 minutes, to make my curiosity happy!

If You like some other way of doing that chat vid-conference, whatever... I am happy too.

Cheers, Florian!

fkuep commented 1 year ago

Coverage badge run succeeds!

nedbat commented 1 year ago

Thanks!!