Making SIA2 discoverable as intended by the standard.

[msdemlei]

This introduces a new sort of syntax for the Servicetype constraint, as the "new-style" standard ids must be matched ignoring the minor version. If further standards follow SIA 2's pattern, we'll have to think about refactoring this. For now, I'll argue for returning to the previous pattern.

With SIA2, there's the additional problem that the authors re-used the SIA1 main identifier. That's a mortal sin as far as our standard id -> service class resolution is concerned. We hence fix this on digesting the query results coming from the registry. I'm pretty sure nothing like this will happen again. Let's consider it a fluke on SIA2.

This commit as an integration test-type test at the end of test_regtap. Perhaps we should move this to test_sia2_remote and replace one of the rather slow tests there? I'd like to leave that call to someone else.

[msdemlei]

Oh gosh... I just saw the CI report and... can I haggle a bit about turning on all of flake8? Most importantly, I personally consider "visual indent" an abomination (and I'm not alone in this), an abomination on top that's making it a whole lot more difficult to write code that stays within 80 characters/line (which I do care about a lot), and that tends to cause unnecessary diff pam. So... can we please drop E128?

Note that I'm not arguing that others must not do visual indentation if they prefer that. Yes, that will lead to a mix of indentation styles, but as long as there is no automatic re-formatting, I don't see how that can badly hurt.

Then, there's a lot of E251. I'd argue there are good reasons to violate it now and then. My use case is that I'm having some large constructors calls with complex arguments in my tests, like

_makeRegistryRecord(
            access_urls = ["http://sia2.example.com", "http://sia.example.com"],
            standard_ids = [
                "ivo://ivoa.net/std/sia#query-2.0",
                "ivo://ivoa.net/std/sia"],
            intf_roles = ["std"] * 2,
            intf_types = ["vs:paramhttp"] * 2)

I'll not quarrel here as much as about visual indentation, but try these this way and without blanks around the "=", and I think most people would agree that in this particular context the blanks help understanding what's going on. Conversely, I don't think readability suffers when some initialisers have blanks around their equal signs and some don't. So... can we drop E251, too?

E226... well... I'm agnostic there, though I will mention that with the lists in the example above I kind of like the extra blanks. But then it seems flake8 doesn't worry about them either.

[bsipocz]

I like the extra info the indents offers, but happy to add E128 to the skip list.

As for keyword/param spaces, I think the distinction is useful. btw, you example I think reads better if everything is aligned to the parens rather than just somewhere:

_makeRegistryRecord(access_urls=["http://sia2.example.com", "http://sia.example.com"],
                    standard_ids=["ivo://ivoa.net/std/sia#query-2.0",
                                  "ivo://ivoa.net/std/sia"],
                    intf_roles=["std"] * 2,
                    intf_types = ["vs:paramhttp"] * 2)

Any particular reasons for keeping strict about 80char lengths? Being more flexible about longer lines would do a lot of heavy lifting of removing weird-looking/forced chopped-up lines.

[msdemlei]

On Fri, Feb 24, 2023 at 10:12:45AM -0800, Brigitta Sipőcz wrote: I like the extra info the indents offers, but happy to add E128 to the skip list.

I'd love that!

As for keyword/param spaces, I think the distinction is useful. btw, you example I think reads better if everything is aligned to the parens rather than just somewhere:

Ok, I'll remove the extra spaces. And <looking indignantly> of course I have to contradict the "somewhere" (it's just one tabstop in almost always). Ahem.

Any particular reasons for keeping strict about 80char lengths? Being more flexible about longer lines would do a lot of heavy lifting of removing weird-looking/forced chopped-up lines.

Well, I come from the school that claims overly long lines are hard to read. I give you that "overly long" does not start at 81 characters. But then one has to choose *some* limit, and the 80 chars convention has been around for a long time and IMHO served us well. Changing the width of my editors and terminals to, say 132 chars would certainly not be entirely unreasonable, but it would eat up screen real estate without buying *me* a lot. But I wont balk when lines in other peoples' code are somewhat longer (on an X display, terminals and editors are quickly made wider), so I'm not advocating any style check to that effect. But I think we shouldn't force coding styles that make it really hard ("chopped-up") to stay within 80 chars either.

[codecov]

Codecov Report

Merging #428 (90418d5) into main (f10aff5) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #428      +/-   ##
==========================================
+ Coverage   79.81%   79.92%   +0.10%     
==========================================
  Files          52       52              
  Lines        5989     6011      +22     
==========================================
+ Hits         4780     4804      +24     
+ Misses       1209     1207       -2     

Impacted Files	Coverage Δ
pyvo/dal/sia2.py	78.09% <ø> (ø)
pyvo/registry/regtap.py	79.60% <100.00%> (+0.57%)	⬆️
pyvo/registry/rtcons.py	97.83% <100.00%> (+0.16%)	⬆️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[bsipocz]

Well, I come from the school that claims overly long lines are
hard to read. I give you that "overly long" does not start at 81
characters. But then one has to choose some limit, and the 80
chars convention has been around for a long time and IMHO served us
well. Changing the width of my editors and terminals to, say 132
chars would certainly not be entirely unreasonable, but it would eat
up screen real estate without buying me a lot.

I'm sorry for jumping in the bikeshed only, rather than reviewing content for this PR: we have 110 in the config files for a while now, that number feels to be a good middle ground to me, still real estate friendly without binding us to limits set by punch cards. It would also let us not do any of the forced and awful \ line breaks in the middle of code lines and in tests so a tiny help with the overall readability.

[msdemlei]

On Mon, Mar 06, 2023 at 09:56:23AM -0800, Brigitta Sipőcz wrote: I'm sorry for jumping in the bikeshed only, rather than reviewing content for this PR: we have 110 in the config files for a while now, that number feels to be a good middle ground to me, still real

Sure, I'm fine with almost anything, just so long as I don't have to do visual indentation :-) Just for transparency: With the force-push yesterday, I'm now configuring tox's flake8 with

…

--ignore=E128,E131,W503 (where W503 is from flake8's default list). Can you live with this? And can you live with this going in in a non-admin commit?

[bsipocz]

Don't do it this way, but add them in the ignore list in setup.cfg, so there will be no errors for local flake8 runs either.
Nitpick: I think it's good to logically separate commit contents (e.g. with your other recent PR it would have been a bit easier to just cherry-pick the test commit for regression testing on main), but this is very much down to personal preference, and I'm not one to enforce my preference (unless a PR incrementally adds zillions of commits with irrelevant contents for e.g. code debug, or add and remove files, none of which is really an issue here in pyvo)

[msdemlei]

On Tue, Mar 07, 2023 at 10:48:25AM -0800, Brigitta Sipőcz wrote: Nitpick: I think it's good to logically separate commit contents (e.g. with your other recent PR it would have been a bit easier to just cherry-pick the test commit for regression testing on main),

I think I agree in principle, but in this case I wasn't sure I wanted to see a commit "now flake8-ignoring E428" in main's history and decided against. But I can easily be swayed on that.

[bsipocz]

super minor comments about the tests, otherwise this should be good to go.

And I suppose it woudln't hurt to add the PR number to the existing changelog entry.

[bsipocz]

what other warnings are expected here?

[bsipocz]

Thanks @msdemlei!