#black-formatter
late dewBOT
bright glacier
and this broke a bunch of tests somehow
deft marsh
Hello everyone
I am learning html
I want to create a game using python for my exhibition
can someone help?
errant barn
You might start from #python-discussion, #game-development or one of the help channels if you have a specific issue. This channel is for Black development
late dewBOT
It has arrived!
flat krakenBOT
I like it! This is getting closer to merge!
This does look a little bit weird, now I see why you originally chose to log the SRCs and the discovered project root in the same line as the project root would provide the necessary context. I don't know if this is worth fixing but I wanted to make sure you're aware.
Finally, I don't know how I didn't realize this but pleeeeasee ...
bright glacier
re. psf/black#2480 -- is black actually adding a comma making the deletion target a tuple 👀
bright glacier
wait no, it isn't -- gosh dammit I fell for the same "commas don't make tuples, parentheses do" trap 😅
dense jungle
hm that might just be something missing in our special case in the ast comparison function
del a, b and del (a, b) have a different AST but are functionally equivalent
so the ast comparator has a hack to treat them as equivalent
del (
[
(),
],
[T],
(),
)
though we shouldn't be exploding this anyway, no?
dense jungle
we should, there's a nested magic trailing comma
bright glacier
hmm, where exactly ...? 
dense jungle
[(),]
bright glacier
right.
tired shard
!remind 2d
late dewBOT
tired shard
@bright glacier Yeah, regarding your path issue the ../../etc shown, even I had seen that in my testing, I didn't raise it thinking you are aware of it, so should I bring back relative src logs?
bright glacier
I don't really see a problem with it 🤷
I'm confused to what you're referring to honestly
tired shard
earlier, we had relative paths in these logs where they would be relative to project root right?
bright glacier
Yes and so what?
tired shard
so i was thinking of having them back, to make the src logs simpler to read like you mentioned in your review
bright glacier
where did I mention that..?
tired shard
This does look a little bit weird, now I see why you originally chose to log the SRCs and the discovered project root in the same line as the project root would provide the necessary context. I don't know if this is worth fixing but I wanted to make sure you're aware.
this? atleast that's what i understand from this
though it won't really be a problem, since ti would only look like this when the user pass paths with ../..,etc
bright glacier
Ah I was unclear there, I was more implying that I now understand why you put the SRCs line and identified x as project root line as one, so it's clear they're related
I wasn't referring to the "reformatted X ..." lines at all
tired shard
ah ic
bright glacier
I probably shouldn't have brought this up then since clearly I'm wayyy too sleep deprived (and generally miserable) to get points across clearly.
tired shard
haha thats mine, clear now 👍
bright glacier
I literally had a breakdown today, I'm not okay fwiw 🤷
tired shard
ouch, i mostly just go on a walk if my days go like that 🤷♂️
@bright glacier sorry one more thing, regarding your last comment about adding a try.ecept handler while finding normalized paths, should i jsut use normalize_path_maybe_ignore and add a option to no_report?
or make report optional
if none, no report
bright glacier
seems reasonable to me although I'm a bit worried people being confused at why a particulier SRC is missing but honestly I can't think through the potential edge cases right now -- I'm not well enough
bright glacier
I don't know
tired shard
ah ok 👍
plush island
Ahh - never heard of black before.
tired shard
<@!429445826251587616> sorry one more thing, regarding your last comment about a...
oh btw, if any of the maintainers has some opinions on this
plucky tundra
Hello there, been curious if there has been a fix towards pep 8's E203 (a: b)
plucky tundra
I mean because currently black formats like a : b
plucky tundra
Oh I see, aight thanks I'll try to make the exceptions in the .flake8
Although I'm curious about something, when I'm using poetry, how do I run black so that it keeps in mind the information that has been provided in the pyproject.toml ?
I currently use black . --target-version py310
But not sure if there's a way to make it recognize poetry's configurations.
little parrot
target version is supported in pyproject.toml
plucky tundra
yeah, it's just that when I use black ., it doesn't seem to use the parameters I've specified at the toml file
Oh nvm, it was a mistake in my file, thanks however
bright glacier
Yes?
bright glacier
making me look a mad man 👀
plain atlas
👀
austere lava
my magnum opus is out on public display! https://github.com/Instagram/LibCST/pull/566
little parrot
did you write your own peg parser or did you use one that already exists for rust?
oh i see it peg
dense jungle
my magnum opus is out on public display! https://github.com/Instagram/LibCST/pul...
congratulations!
muted haven
my magnum opus is out on public display! https://github.com/Instagram/LibCST/pul...
I totally forget about this, it is an amazing work 🎊
bright glacier
🎉 félicitations!
austere lava
Ty ty, feel free to leave comments even just on parts of it
I know it's unrealistic to expect a full review
Also beware of 
mint barn
👀
nova tinsel
@left vine that's not at all appropriate for this server. please refrain next time.
tight citrus
bronze iron
same
frigid grotto
could my pr (2698) get a review :)
bright glacier
It seems fine to me but I'd like Jelle to review it as they're the one who opened the issue. (I can also run your PR against a suite of projects to help determine the impact)
It's probably worth changing https://black.readthedocs.io/en/latest/the_black_code_style/current_style.html#comments to note how we normalize type: * comments
hmm, does typed-ast not recognize type comments that aren't perfectly well formatted? I suspect that's why the AST check is failing on Python 3.7 and below where Black hard depends on typed-ast.
frigid grotto
makes sense, ig for that i would add a version specific unit test instead of changing the general ones?
or is there a better way to deal with that
bright glacier
No we're gonna have to do something more major because it's not just the tests that are failing, this code will cause black to fail with an AST safety error IRL on 3.7 and below
frigid grotto
i see
dense jungle
hm if typed-ast doesn't recognize one version of the comments then we shouldn't be changing this, because it will change the meaning of the code (to a typechecker)
bright glacier
Does mypy still support Python 2? If not, I suppose once 3.7 goes EOL we could maybe normalize type comments although that is a loooong time away.
bright glacier
I'm happy enough with this current basic summary as it includes enough information to be helpful (and I have a headache anyway 😦 )
stuck vapor
I use pyupgrade for that
same, in every project 😆
plain atlas
I use pyupgrade for that
pyupgrade can lose performance enhancements ;-;
red sedge
?
bright glacier
pyupgrade did revert its "convert list comprehensions to generators" change so that minor perf hit is gone fwiw @plain atlas -- there's probably more that you know but I'm pretty sure this is the main one
LOL -- the diff-shades project is being formatted by black v21.9b0 
sorry Jelle
dense jungle
LOL -- the diff-shades project is being formatted by black v21.9b0 <:lemon_sweat...
how are you going to get enough newlines around your nested functions
(I think that's the only formatting difference from the last few months)
bright glacier
actually? I thought we had more
dense jungle
unless you're using 3.10 I guess
bright glacier
You're right reading the changelog, all of the rest have been feature additions or bugfixes 👀
dense jungle
It makes upgrades easy 🙂
late dewBOT
!remind 2d
It has arrived!
tired shard
@bright glacier Do you basically want me to write tests for normalize_path_maybe_ignore utility function since that's the only function handling the file paths here
Re: https://github.com/psf/black/pull/2526#pullrequestreview-834698329
bright glacier
I was more implying that you test the output since I would've assumed normalize_path_maybe_ignore already has tests (if not, oh dear) under various conditions.
tired shard
it doesn't
bright glacier
sigh. well OK if you don't want to test the output then at least testing normalize_path_maybe_ignore would be good enough.
tired shard
yeah, cuz the output basically depends on that
flat krakenBOT
dense jungle
fyi I fixed the merge conflict for you, in case you encounter it locally 🙂
tired shard
i was gonna fix that at the end, cuz it would appear again if another pr gets merged
if we had better changelog handling :D
bright glacier
.. while I'm working on getting psf/black#2095 merged since clearly our change in idea is well liked
toxic stormBOT
bright glacier
if we had better changelog handling :D
We could use towncrier fwiw although it might bring too much complexity
dense jungle
Honestly I don't mind it too much, the merge conflicts are easy to fix
Though I've seen a few cases where a PR ended up with its entry in the wrong section
tired shard
i am writing one for modmil, almost complete, its pretty simple - works like the one at cpython
bright glacier
I'm done actively developing diff-shades btw, now once I get PR 2095 dealt with I'll look into integrating it into a GHA workflow.
tired shard
btw, while adding tests in test_black.py, is there an order, or i just add it to the end?
tired shard
I'm done actively developing diff-shades btw, now once I get PR 2095 dealt with ...
that would remove all the colours 
bright glacier
btw, while adding tests in test_black.py, is there an order, or i just add it to...
Unless there's already present test class (eg. TestFileCollection, TestCaching) or its own file for the area of concern it doesn't matter as it hasn't been reorganized since it was created back in 2018 :)
bright glacier
that would remove all the colours <:lemon_pensive:754441880246419486>
Nah, it's pretty easy to force colours w/ rich
tired shard
on gha?
bright glacier
And even if I couldn't get 'em back in GHA, rich has this super cool feature where you can export a HTML file that's similar to what was emitted to the console. Though it's far from perfect.
tired shard
ah yes, perfect
tired shard
Unless there's already present test class (eg. TestFileCollection, TestCaching) ...
ah okay, there's TestFileCollection
tired shard
ah okay, there are tests for normalize_path_maybe_ignore but they test it indirectly
like test_symlink_out_of_root_directory
dense jungle
Unless there's already present test class (eg. TestFileCollection, TestCaching) ...
Maybe we should split it up in parallel with the file splitting in src/black
tired shard
https://github.com/ichard26/diff-shades/runs/4570930067?check_suite_focus=true#s...
wow, lovely
bright glacier
Maybe we should split it up in parallel with the file splitting in `src/black`
Yeah that was an original goal but no one has worked on that yet. I can open a ticket about a larger test suite refactor in particular splitting the cases up even more to follow the modules
bright glacier
wow, lovely
I'm pretty proud of diff-shades to be honest since it's probably the most complex application I've written 100% myself -- the feature set and the polish is way beyond what I usually target :)
bright glacier
I did mention by name as KJ starred it with incredible timing in the staff lounge, yes.
tired shard
ah
bright glacier
I'm pretty proud of diff-shades to be honest since it's probably the most comple...
The README does a decent job at detailing all of the main features diff-shades has: https://github.com/ichard26/diff-shades#readme
tired shard
ah okay, there are tests for `normalize_path_maybe_ignore` but they test it indi...
ok getting back to writing tests ^
u oh, failing tests, need to add ctx.obj
tired shard
ok tests fixed
bright glacier
I on the other hand broke eight tests =p
tired shard
i broke more lol, cuz ctx.obj wasn't present
added a dmmy root since msot don't care
and added the real root for those which depend on it
tired shard
ah okay, there are tests for `normalize_path_maybe_ignore` but they test it indi...
btw richard about this, should i write tests for the output
bright glacier
Uhh, if you want to?
As long as we don't let any OSErrors / ValueErrors leak through unhandled it's probably fine
tired shard
ok, what should they be like, since imo the output depends on the normalize function, which is already tested
bright glacier
Perhaps checking this logic would be good? else f'\033[31m"{source} (skipping - invalid)"\033[34m'
it doesn't seem to be part of the normalize function
although I 100% skimmed the diff and could've easily missed something
tired shard
not sure, that's never really gonna change and it works right now
bright glacier
Alright then no extra tests then. The normalize func is well tested now, right? If so I'm happy
tired shard
there is a single test for symlinks, i can try writing one for file perms if that would be possible
bright glacier
That'd be nice although that sounds finicky with OS dependencies across the wall
bright glacier
I'm not surprised.
bright glacier
❯ diff-shades compare more-consistent-spacing-compiled.json hug-power-op-compiled.json
[21:07:33] Loaded first analysis: /home/ichard26/programming/oss/black/more-consistent-spacing-compiled.json
Loaded second analysis: /home/ichard26/programming/oss/black/hug-power-op-compiled.json (cached)
╭───────────────────────── Summary ──────────────────────────╮
│ 13 projects & 254 files changed / 1748 changes [+874/-874] │
╰────────────────────────────────────────────────────────────╯
Differences found.
alright not too bad for hugging power operators
although I did realize a mistake in my logic looking at the diff
bright glacier
OK, having looked through the whole 8000 lines diff, there's three main things that stick out:
- proxy = lazy(lambda **kwargs: NumberAwareString(), NumberAwareString)(**kwargs)
+ proxy = lazy(lambda**kwargs: NumberAwareString(), NumberAwareString)(**kwargs)
- self.assertAlmostEqual(obj.big_power, obj.big ** obj.small)
+ self.assertAlmostEqual(obj.big_power, obj.big**obj.small)
- return super(B, self).f ** 2
+ return super(B, self).f**2
The first is obviously a bug, but the other ones stuck out as cases where potentially the hugging shouldn't happen. Thoughts @dense jungle ?
The third one should probably not hug but I'm not sure how I'd implement that other than taking the easy route which is banning attribute access too but that's debatable.
dense jungle
OK, having looked through the whole 8000 lines diff, there's three main things t...
I'd be OK with (2) hugging
(3) probably not. Is it easy to recognize when the LHS of the attribute access has a function call in it?
bright glacier
Unfortunately I'm just working with a flat list of Leaf objects so there's no node information whatsoever as that's how the transformer infrastructure works
There's maybe a hack I could do to make this work
tired shard
only keep double star operators without spaces when they are literals and not vars/objects?
bright glacier
only keep double star operators without spaces when they are literals and not va...
that would mean a**b gets transformed to a ** b which seems suboptimal
tired shard
hmm, yeah
bright glacier
also, man is diff-shades slow when trying to quickly iterate on changes on my several years old low-end dual core CPU
tired shard
But i kinda like 2 and 3 with spaces 🤷♂️
bright glacier
but the data it provides is invaluable so I shall run it :)
tired shard
its easier for me to recognize the two
bright glacier
But i kinda like 2 and 3 with spaces 🤷♂️
eh, I'd agree on its own it looks better not-hugged, but IMO it looks better once it's part of a bigger expression, e.g. offset + ctx.main**ctx.other vs offset + ctx.main ** ctx.other
!rule 5 - Probably not as that sounds malicious.
late dewBOT
lucid mesa
quiet kraken
!mute 852911179450023976 I think you need a timeout. Please respect our rules.
late dewBOT
:incoming_envelope: :ok_hand: applied mute to @lucid mesa until <t:1639889572:f> (59 minutes and 59 seconds).
bright glacier
For the record @lucid mesa , this channel is for discussion of the psf/black project, not any random OSS project. You can request to the admins the creation of a channel for your own OSS project but there's a big list of items it needs to filfull.
tired shard
its filfull or fulfil
bright glacier
fulfill
And before you ask "why not don't hug when it's the only operator in the expression?" well that's hard to implement
potentially doable but I don't want to implement that level of hackiness today
tired shard
eh, I'd agree on its own it looks better not-hugged, but IMO it looks better onc...
hmm, for recognizing the two I think (2) is better, for understanding the expression like offset is getting added to main**other (1) is better.
Ah ok, i was gonna ask that (:
bright glacier
I'm sorry but what you just said is incomprehensible to me
tired shard
uhh like when it is main**other in the comprehension, it's not really recognizable, like main is getting raised to other when you are skimming through it (in bigger code), but while reading code properly I find main**other is more "understandable" cuz it groups the expression like you are raising main to other first and then adding offset
bright glacier
I still don't get it honestly. The only hope you have at having me understand is using complete code examples. Sorry.
tired shard
offset + ctx.main**ctx.other
``` basically, i find this easier to understand, like main is getting raised to other and then offset gets added while
```py
offset + ctx.main ** ctx.other
``` This is more prettier to look, and the `main ** other` is more recognizable when you are skimming throughAh that makes sense, thank you for clarifying!
dense jungle
Honestly I feel like we might need feedback from people who often write math-heavy code
tired shard
you could probably make something like a survey, put it on twitter/github/discord?
glacial gorge
!e lst = [1,1,1,2,2,2,3,3,3,4,4,4,5,5,5,6,6,6]
D = 3
tuples = tuple(tuple(lst[i:i+D]) for i in range(0, len(lst), D))
print(tuple(tuple(tuples[i:i+D]) for i in range(0, len(tuples), D)))
boreal nova
I forked the project and I keep getting ModuleNotFoundError: No module named '_black_version' when trying to run tests.
anybody know a fix?
bright glacier
I usually get something similar when I switch commits before and after our major refactoring. Try a clean reinstall python -m pip uninstall black -y and python -m pip install $however-you-installed-black-in-the-first-place
Wait, how are you running black then?
boreal nova
I usually get something similar when I switch commits before and after our major...
Black is not installed as I forked the project
I downloaded the dependency requirements for testing and in setup.py
bright glacier
I'm pretty sure we don't support running black in-tree but I can quickly test right now
boreal nova
Awesome thank you
I have an idea for the project and I want to attempt to implement it
bright glacier
Yeah no, we don't support it as we use absolute imports everywhere (almost no relative imports)
You're gonna have to install it (an editable install should work fine)
bright glacier
That's good, good luck on modifying black! 🌺
boreal nova
Now that you're here actually I have a quick question that would save me from diving deep into it
if you don't mind
bright glacier
Sure, what's the question?
boreal nova
Where exactly are files being read and then parsed from?
Read (Looking for this aspect) -> parse -> output
Keep in mind I just forked it and started looking at it so if I'm wrong in how it works my apologies 😄
bright glacier
Hmm, what are you planning to contribute?
But we read the file in black.format_file_in_place IIRC
boreal nova
Alright awesome, I can run my idea by you and it'd save some time I guess if it makes sense or not
I have some python examples in my README.md that I put in a while ago. I want to format them but it would be a pain to copy and paste all new examples as theres a pretty good amount. I'm gonna see if I can get black to read python code within files that are enclosed in a python block.
little parrot
use blacken-docs for this
boreal nova
Unless it can already do that lmao
bright glacier
I doubt we would accept this in the authoritative version of black but I'd be surprised if there isn't a fork of black that supports this already lol
little parrot
bright glacier
use blacken-docs for this
Oh, does blacken-docs support MD? I thought it was RST only.
dense jungle
This actually seems like a reasonable thing to support in Black itself
bright glacier
It would definitely have to be opt-in as most people run black via black . and we don't want to suddenly reformat their docs folder :p
boreal nova
As it is my idea it would be greatly appreciated if I could have a chance at developing this. However for the sake of open source and you guys having more experience working with the lab I can hand it to you if this is going to be a future plan now that I mention it.
boreal nova
It would definitely have to be opt-in as most people run black via `black .` and...
It can be a command paramater
such as black . --include_md
or something similar
or --include_md or --include_(other doc filetype)
dense jungle
The most similar thing we do right now is ipynb files, as I recall there's no flag for them but you need to install an extra
bright glacier
I'm already wary of the complexity the jupyter support brought so I'm hesitant to say yes especially as blacken-docs exists but I'm just one opinion out of 9 ... or 10?
boreal nova
Worst case I'll bring it to the table and you can deny or merge my pull request.
I do this for fun so either way it's all good 😄 I am just trying to get more involved contributing in well known libraries
A heads up would be nice as if this would to be accepted at all before I really get into it
bright glacier
It's not so much the actual technical complexity I'm more worried about, it's just more the maintenance burden I'm worried about. Like we have an official vim plugin but none of the maintainers can write vimscript so almost all third-party plugins are much better lol.
I'm sure Jelle would accept but I'm not so sure.
errant barn
I think this would be a good place for a wrapper. But I'm not opposed to including it in Black.
bright glacier
I do this for fun so either way it's all good 😄 I am just trying to get more in...
Looking at the jupyter PR https://github.com/psf/black/issues/2345 should help for some pointers
that's not the PR, that's the issue oops
boreal nova
Awesome, thank you
Running black in docs wouldn't require a separate install or really much technical complexity as it's just docs (or so I believe) compared to jupyter notebook.
bright glacier
Well yes, but it's more the maintenance expectations I'm worried about as I already stated. We're still gonna have to pull in MD parsing libraries probably (and I suspect we'll get feature requests for RST too). Testing optional features is quite annoying (we have to rerun the test suite for each feature on/off).
boreal nova
Well yes, but it's more the maintenance expectations I'm worried about as I alre...
I'll write tests as well and I'm hoping to accomplish this without adding dependencies
bright glacier
But then again, perhaps it'll look less scary once there's a basic functional implementation
boreal nova
But then again, perhaps it'll look less scary once there's a basic functional im...
Yeah for sure, I'll work on getting something to look at when I can
I'm motivated to work on this as it'll make my life a lot easier as well lmao
Thank you guys for being supportive. 😄
bright glacier
I'll write tests as well and I'm hoping to accomplish this without adding depend...
I wonder how fast it'll take for people to ask for RST and myst support (they both have nested codeblock support 🙃 )
what's the xkcd about evergrowing expectations? lol
bright glacier
I wonder how fast it'll take for people to ask for RST and myst support (they bo...
We can cross this bridge when it does happen btw.
errant barn
Yeah definitely RST if we're doing this :D
bright glacier
and that means adding another extra for RST support -- which means at this point we might want to redo our testing setup and it's already a bit annoying with our python2 and jupyter extras
errant barn
Too bad Sphinx tooling can't help since we want the formatting to the source files as well.
A formatter extension would be quite trivial to make. Please don't quote me 😅
bright glacier
I don't like how we have to rerun the whole test suite (or a good portion of it) with reinstalls and uninstallations of dependencies but I guess that's the unfixable complexity of a large OSS application
Unless there's some hackiness we can abuse in the importlib machinary to block certain imports .. (for the record, I have never looked into importlib)
errant barn
That could be interesting, since multiple installs seem pretty awkward. Fortunately no extras vs. all extras should suffice, since they wouldn't really interact (heh, surely not right, right?)
bright glacier
Well that's the hope but extras like Python 2 does show it's possible for inter-extra interaction
(thankfully we're killing that next month)
errant barn
Yeah, my first instinct would be to let Black for docs be a wrapper since there's a clear divider between Python source and whatever else people use for docs or even other things. But on the other hand, docs are pretty common and we already support IPython too so..
bright glacier
Yeah yeah I see that point and it's that slippery slope I'm actively fighting against because I already feel like there's enough development work to be done 😄
I suppose I'm not sure what else other than docs we would get requests for though. Other than the several documentation formats that exist out there
Markdown, MyST, ReStructuredText are the ones I know off the top of my head
!d importlib.util.LazyLoader wait what this exists?
late dewBOT
bright glacier
this isn't useful for this situation but I'm surprised they managed to make a clean enough implementation to put one in the STDLIB
hoary spire
Black is a great formatter. It's even more good when you use with pre-commit
bright glacier
Thank you! 
late dewBOT
@meager rampart Please don't try to ping @everyone or @here. Your message has been removed. If you believe this was a mistake, please let staff know!
vocal escarp
I gotta admit, this thing on the Google home page confused me for a moment today:
tired shard
bonk richard, review on enhance verbose pr :D
bright glacier
bonk richard, review on enhance verbose pr :D
!remind 5H
late dewBOT
late dewBOT
!remind 5H
It has arrived!
bright glacier
!remind 2H I'm literally housekeeping right now
late dewBOT
late dewBOT
!remind 2H I'm literally housekeeping right now
It has arrived!
flat krakenBOT
bright glacier
I haven't accepted a feature request so fast in such a long time 😄
tired shard
even colouring logs would be helpful :)
bright glacier
ok now you're just asking for an opportunity to make another PR :)
tired shard
lol not exactly, but i had included that in enhance verbose pr earlier
and felix liked the idea but told it was out of scope and wanted other maintainer's comments, didn't open an issue after tht
also you can add the command line label on that issue
plain atlas
ok now you're just asking for an opportunity to make another PR :)
imagine pacing yourself
can't be me
short forge
logging feature requests in this day and age?
better read that pull request extra closely
errant barn
Btw, has the Black project been audited for log4j vulnerabilities yet?
lament crow
Btw, has the Black project been audited for log4j vulnerabilities yet?
Shouldn't take more than one minute to check if Java shows up as a language for black project :D
errant barn
Shouldn't take more than one minute to check if Java shows up as a language for ...
We should check it just to be sure
Hmm, looks like we might be alright.
lament crow
Time to charge for the audit that you've just performed
errant barn
I could do this for a living I bet
flat krakenBOT
bright glacier
lol I was going to merge that
flat krakenBOT
dense jungle
lol I was going to merge that
got to be faster 😄
bright glacier
you know that's my weakness :p
muted haven
Seems like this is the last grammar change we need in order to fully parse all written examples from the CPython. I thought my previous PR was going to do that, though it seems like the script that I use to test was missing a few cases.
flat krakenBOT
muted haven
RE the peformance stuff: I still have not found a reliable way of implementing in the way that I originally did in the parso, since from what I understand, parso's internals have evolved quite a bit on top of the lib2to3 in terms of pre-computation of transitions. I still have a few other ideas, but at the worst choice we could hard-code this hack instead of a backtracking parser. I also am not so sure how much time should be dedicated into this if the parser change stuff is still on the short-term plan.
dense jungle
RE the peformance stuff: I still have not found a reliable way of implementing i...
Thanks! I think we're unlikely to make a parser change any time soon, because the big motivator was that we thought we wouldn't be able to support match with blib2to3. Since that's now fixed, there's no strong reason to switch any more.
muted haven
Good to know 👀
errant barn
BIG
errant barn
oh. 😅
errant barn
will do, sec
bright glacier
@tired shard feel free to open an issue to add more colours to the output btw
bright glacier
@plain atlas what does continue-on-error do btw? I know it allows the job to continue executing even if the step fails but does it mark the job as failed still? If so great!
stuck vapor
Minor point, but in the original issue the OP had written
Since you labelled it as a good first issue, I would love to work on it and make a PR
, hopefully they don't feel like this PR-opportunity was "stolen" from them 😆
(though from my experience in pandas, when people write that they want to submit a PR, they rarely do, please don't take this as a criticism for having accepted the PR, I think it was the right thing to do!)
bright glacier
Yeah that made me sad to see someone else steal the issue 
plain atlas
<@!717983911824588862> what does `continue-on-error` do btw? I know it allows th...
it does not mark the job as failed, no. Although that can be done with a custom step to check the outcome of said step, and exit with 1 if it failed.
or does it
uh
bright glacier
Alright, IIRC that was its original behaviour, no? Thanks for the info -- I'm finally putting together the diff-shades workflow
plain atlas
it does not mark the job as failed, no. Although that can be done with a custom ...
i'm wrong
wait no
that's different thing
bright glacier
So I was right? Sorry I'm kinda buried in this workflow stuff and I can't stuff well
plain atlas
it does not mark the job as failed, no. Although that can be done with a custom ...
this is right
plain atlas
https://github.com/discord-modmail/modmail/runs/4327233675?check_suite_focus=tru...
however, if you want to continue after it, you can use expressions, like I did here
stuck vapor
I think there's also fail-fast if you want it to continue after a failed step but still mark the whole workflow as having failed?
bright glacier
@stuck vapor btw diff-shades is basically my version of mypy_primer so enjoy (once I get this workflow finished 😆 )
plain atlas
I think there's also `fail-fast` if you want it to continue after a failed step ...
fail-fast is for distributed matrix testing, where if one fails, to stop running all of them. Eg you have the tests on 20 different machines, if one fails, all other are aborted
stuck vapor
fail-fast is for distributed matrix testing, where if one fails, to stop running...
ah, right - thanks!
stuck vapor
<@!633587720270381077> btw diff-shades is basically my version of mypy_primer so...
nice, well done!
bright glacier
hi @plain atlas for github.event_name what happens for pull_request events as it has subevents?
plain atlas
:glare:
bright glacier
Hopefully it's still pull_request?
plain atlas
idk uh lemme find it
plain atlas
Hopefully it's still `pull_request`?
bright glacier
alright
bright glacier
you don't know how much work this took lol -- anyway it's coming along pretty nicely so far :)
bright glacier
I did not expect this to be so complicated 😅 .. and I'm not even done yet
bright glacier
hey, a basic test run worked!!! https://github.com/ichard26/black/actions/runs/1609022299
now I need to test the PR, custom revisions, and custom black arguments features :)
plain atlas
looks good
bright glacier
this is the most bananas helper script I've ever written https://github.com/ichard26/black/blob/f8ec04ad4ddcf1cf6ad9fec07c44f8ca8d4c1b50/scripts/diff_shades_gha_helper.py
bright glacier
Custom arguments (and the workflow_dispatch event) works too: https://github.com/ichard26/black/runs/4601156117?check_suite_focus=true#step:12:1 🎉 although clearly I need to fix this line wrapping issue
bright glacier
looks good
You know I was expecting you to have a million pieces of feedback given you're basically the devops person I know
plain atlas
well, a, I'm tired, and b, isn't Joe more devops than me!?!
bright glacier
OK that's true.
flat krakenBOT
plain atlas
^ blacken-docs exists
tired shard
<@!736234578745884682> feel free to open an issue to add more colours to the out...
!remind 2d
late dewBOT
tired shard
also enhance verbose 🙃
bright glacier
Status update: PRs and some caching has been successfully added to the diff-shades workflow, the last thing left to do is add the comment post/edit feature.
The flipside is that this integration is now quite complex! It's for good reason though as this is feature packed and hopefully pretty well polished.
The main reason why this is so complex is that the workflow supports three modes of operation, two of them exist to resolve our needs defined in psf/black#1390. The other one is a workflow_dispatch, i.e. basically you can run diff-shades against arbitrary revisions/branches/tags (including the latest PyPI release and a PR!).
dense jungle
**Status update:** PRs and *some* caching has been successfully added to the dif...
Thanks for all your hard work! This is going to be very useful
bright glacier
hmm, I'm in bit of a limbo when it comes to the comment feature, the workflow_run event seems ideal but I'm not sure whether it's possible to know what event triggered the original workflow run and while workflow_dispatch works around this issue, I run up to the permission limitations of the default github.token
I could just abuse the blackrelease github user and use a PAT but I'll see if I can make workflow_run work
https://github.com/github/docs/issues/799#issuecomment-795352578 Oh if these so-called "extra properties" contain the triggering event that'd be awesome
tired shard
you can use github.event.name or github.event.type ig
if i understand what you are saying correctly
bright glacier
you can use `github.event.name` or `github.event.type` ig
does that work from a secondary workflow run triggered by the workflow_run event?
tired shard
never tried it 🤷♂️ , it basically tells how was the action triggered, like was it because of pull request edit, was it cuz of workflow dispatch, etc
bright glacier
Yeah but I need to know how the initial workflow was triggered, not this secondary one
tired shard
wdym by initial and secondary
bright glacier
so the workflow_run event allows you to have a second workflow run right after a configured workflow completes -- basically it's a highly isolated way of doing staged builds
also screw the GHA docs, I'mma read the github API docs which seem to be better maintained anyway
tired shard
ah right got it, so say you run it after linting workflow, then you wanna know how the linting workflow was triggered?
bright glacier
Yup and annoyingly enough I can't find any way to do that even with using the Github API
poor soul, in the same limbo as me: https://stackoverflow.com/questions/69517510/workflow-run-only-on-push
tired shard
probably, can we get the workflow ID of the initial workflow somehow?
bright glacier
actually I have a solution but I hate how hacky it's going to be
tired shard
which is?
bright glacier
Nevermind it won't work
tired shard
what is github.event.actor for secondary workflows?
bright glacier
That's the username which isn't helpful
tired shard
it is the user who triggered the initial workflow?
bright glacier
I don't know and I don't see how it could be helpful as I need to know whether the initial workflow was triggered by a push or a pull_request event
tired shard
if it returned the workflow which triggered the secondary workflow we would have its id, then we could create a simple script to fetch the event triggered the initial workflow
bright glacier
Getting the ID of the initial workflow run is easy but
then we could create a simple script to fetch the event triggered the initial workflow
is not clear to me
bright glacier
if it returned the workflow which triggered the secondary workflow we would have...
do you know something I don't because I've been staring at the API docs to no avail
Hmmm, I wonder if this will work: "check_suite[pull_requests] "
it's so poorly documented though that I have my doubts
tired shard
Getting the ID of the initial workflow run is easy but
> then we could create a ...
we use the API to fetch the workflow event with that id
bright glacier
Yeah but how does that net me the triggering event?
That has been my question this whole time if it wasn't clear
tired shard
which would returned workflow event object which contains event type
yeah it was clear 👍
bright glacier
which would returned workflow event object which contains event type
This isn't clear to me whatsoever
tired shard
if we can get the initial workflow ID, we can use the github API to fetch the workflow object on /repos/{owner}/{repo}/actions/workflows/{workflow_id}/runs which w returned the workflows ran for that ID which would also contain the event type which was triggering the workflow
bright glacier
I hate the docs so much
bright glacier
OK I understand now, thank you very much @tired shard
tired shard
👍 np
bright glacier
hopefully there isn't some weird restriction that makes this unusable
plain atlas
https://github.com/github/docs/issues/799#issuecomment-795352578 Oh if these so-...
run a context dump workflow action
austere lava
Anyone has a good way to spot minor differences between two pieces of source code? So far I've been relying on replacing whitespace characters with highly visible unicode characters and just running a simple colordiff.
But I've been staring at this colordiff output for 10+ minutes until I spotted the 2 differences
tired shard
I just use delta, its pretty easy to spot the changes then
austere lava
what's delta?
tired shard
tired shard
np
bright glacier
man, this comment functionality is annoying to write, it's by no means hard but there's so many ways I could do it it's hard to choose :p
@plain atlas is it possible to get the link of an uploaded artifact within the same workflow?
plain atlas
why?
bright glacier
Because I want to include a link to the uploaded diff artifact in the comment body
bright glacier
You know what, I'mma just use the GitHub API since you seem very unhappy lol
plain atlas
might be a bit roundabout so lemme see if there's an action that does it already
bright glacier
I already have the logic to interact with the API so 🤷
plain atlas
You know what, I'mma just use the GitHub API since you seem very unhappy lol
that was the way i was thinking that you could do it ;-;
bright glacier
I don't want to maintain shell scripts as part of this workflow :)
plain atlas
its not a shell script afaik
bright glacier
I know, but I'm deferring most of the complex logic to this helper script which is in Python which is way friendlier to work with
plain atlas
ahhhh gotcha
in that case
@bright glacier there's obviously a way
upload-artifact wraps https://github.com/actions/toolkit/tree/main/packages/artifact which returns an artifact response which has a url to it
bright glacier
well that's annoying because I only looked at the upload-artifact docs >.<
bright glacier
It's fine, I have another solution I'm working towards right now
bright glacier
Yup I am already querying that api endpoint lol
bright glacier
Right I almost forgot I need to make changes to diff-shades itself too
bright glacier
yay, this took more effort than it should've since I am an idiot :p
bright glacier
well what a great start to testing
bright glacier
@plain atlas do you mind helping me with yaml + gha again?
plain atlas
you already know the answer :glare:
why would you even ask if I'm willing to help smh
bright glacier
... I can always ask Joe for help if you're busy
bright glacier
why would you even ask if I'm willing to help smh
I don't want to be that annoying lol
plain atlas
lol
bright glacier
But anyway is it possible to set the result of an expression as a step output?
- name: Check if comment is needed
id: needed
run: >
echo "::set-output name=value::${{ steps.metadata.output.event ==
'pull_request' && github.event.workflow_run.conclusion !== 'success' }}"
i tried this but it failed with
The workflow is not valid. .github/workflows/diff_shades_comment.yml (Line: 33, Col: 14): Unexpected symbol: '='. Located at position 89 within expression: steps.metadata.output.event == 'pull_request' && github.event.workflow_run.conclusion !== 'success'
Hmm, actually I could workaround this by adding even more code to the helper script
plain atlas
do you use vscode or pycharm, i can't remember
bright glacier
Vim
plain atlas
shoot
i use a vscode extension which validates my workflows locally lol
so i don't get syntax errors 🤡
which is what I was going to suggest to you
bright glacier
Lol, I do have vscode but I just never use it ... but I'll gladly use it if it prevents these syntax issues 😄
plain atlas
Name: GitHub Actions
Id: cschleiden.vscode-github-actions
Description: GitHub Actions workflows and runs for github.com hosted repositories in VS Code
Version: 0.21.3
Publisher: Christopher Schleiden
VS Marketplace Link: https://marketplace.visualstudio.com/items?itemName=cschleiden.vscode-github-actions
bright glacier
Thank you
Haha, it's rare when my bugfixes actually reduce the amount of code:
❯ git diff --stat
.github/workflows/diff_shades_comment.yml | 14 ++++----------
scripts/diff_shades_gha_helper.py | 6 +++---
2 files changed, 7 insertions(+), 13 deletions(-)
fun fun fun
plain atlas
typical fix: open task manager and kill a specific windows process datahog
actual: do nothing because i can't get an admin window
bright glacier
do you have any important work open? cause I'd power cycle the machine at that point
plain atlas
do you have any important work open? cause I'd power cycle the machine at that p...
i did power cycle lol
plain atlas
But anyway is it possible to set the result of an expression as a step output?
...
i thiiink so
bright glacier
I'm way past that issue now :p
plain atlas
so what was the answer
plain atlas
nice
bright glacier
I mean, this is progress but I got some bugfixing to do 😄 https://github.com/ichard26/black/pull/4#issuecomment-999826235
bright glacier
Yeah, all of the retries after pushing a probably still broken fix
OK, I fixed one problem but here's another one, what the heck happened here 👀
plain atlas
looks like you appended to the existing content
bright glacier
Yeah, not sure how that happened though.
plain atlas
late dewBOT
.github/workflows/diff_shades_comment.yml lines 42 to 50
- name: Read comment body from helper script
id: body
if: steps.metadata.outputs.needs-comment == 'true'
run: |
body="$(cat .pr-comment-body.md)"
body="${body//'%'/'%25'}"
body="${body//$'\n'/'%0A'}"
body="${body//$'\r'/'%0D'}"
echo "::set-output name=body::$body"```
dense jungle
that's an interesting choice for the default
plain atlas
that's an interesting choice for the default
imo its not. A lot of ppl use actions to add other info to comments made by humans
like in a pull body, it'll add a <details> section on the bottom with internal issue tracking stuff
its a lot smarter to append to already existing stuff than to overwrite everything that's already there
because this way, explicit opt-in to removing everything, vs not knowing all of the content gets removed
bright glacier
plain atlas
suggestion: delete some of these
bright glacier
The only one I'd maybe delete would be the first one as that one is purely internal
plain atlas
i have no idea which i'm supposed to download at first glance, since the 48KB ones are realllly enticing
🥺
well they're downloaded now
bright glacier
Those JSON files are diff-shades analyses
plain atlas
ah
bright glacier
Basically they exist just in case my workflow breaks and I need to debug the generated analyses
plain atlas
well
bright glacier
(also they're useful if you want to compare other revisions without redoing the analyses yourself but that seems rare)
bright glacier
Yeah.
But technically it's better than mypy-primer because it doesn't have a structured output option 😉
plain atlas
plz make coverage.py-like html generator from these json files kthxbai
bright glacier
did you open diff.html?
bright glacier
Yeah if there's any formatting differences then it'll include a diff lol
Perhaps I shouldn't upload the diff if there's no changes 🤔
actually @dense jungle , you can see this example run too and see if it's a smooth enough workflow for you (sadly embedding the whole diff into the comment is too unreliable as we'd hit length limits really fast).
I also wish GitHub Actions would allow you to upload a file without zipping it but oh well
plain atlas
plz make coverage.py-like html generator from these json files kthxbai
might be too complex now that i think of it
dense jungle
I'm in a lot of trouble right now because my cat wants to eat my lunch
bright glacier
what did you get as lunch 👀
plain atlas
inb4 catnip
bright glacier
Yeaa good luck to you then! 🐈⬛
bright glacier
https://nedbat.github.io/coverage-reports/reports/20211113_19bc85a945/htmlcov/d_...
But yeah is the HTML diff generated in that run I linked good enough for ya? I got it pretty for free as rich comes with an awesome "record output as html" feature
dense jungle
actually <@!783088578363523104> , you can see this example run too and see if it...
ok I finished
bright glacier
I could add a fancier output mode to diff-shades but honestly I'm so tired that I don't want to continue work on this lol right now
plain atlas
good enough for me
ofc it is because i won't be using it much lolol
dense jungle
I really like the diff html but it's sad that you have to download + unzip + open locally to see the output
if length limits are a concern, maybe put a truncated version in the comment?
bright glacier
Alright let's see how much work that'll take -- probably not too much given the script is the one driving this anyway
Does one of you by off chance know the comment length limit?
plain atlas
f"""
<details>
<summary>
Partial diff
</summary>
{diff}
</details>
"""
65536
plain atlas
```md
f"""
<details>
<summary>
Partial diff
</summary>
{diff}
</details>
"""
`...
(actually you'll want ```diff``` in here too)
bright glacier
I would make a little web tool (similar to next-pr-number) to avoid the download + unzip steps entirely but I can't find a way to get a publicly accessible artifact download link
but having a truncated diff in the comment is a good idea too
plain atlas
I would make a little web tool (similar to next-pr-number) to avoid the download...
wdym?
bright glacier
So the download link provided in the API is only usable if you pass a OAuth2 token with actions: read permissions
velvet whale
bright glacier
https://www.youtube.com/watch?v=nQC6MyBHceE
Please share random YT links in the OT channels, thanks.
velvet whale
Please share random YT links in the OT channels, thanks.
oh oke
bright glacier
So the download link provided in the API is only usable if you pass a OAuth2 tok...
Ok, correction, this would work if I could get the user to login OAuth2 style with their GitHub account but that simply moves the problem to "where would I host this since I doubt you can do OAuth2 with a static web server"
plain atlas
So the download link provided in the API is only usable if you pass a OAuth2 tok...
think you could provide a link to one of these?
bright glacier
{
"id": 130415329,
"node_id": "MDg6QXJ0aWZhY3QxMzA0MTUzMjk=",
"name": "diff-report.html",
"size_in_bytes": 1051,
"url": "https://api.github.com/repos/ichard26/black/actions/artifacts/130415329",
"archive_download_url": "https://api.github.com/repos/ichard26/black/actions/artifacts/130415329/zip",
"expired": false,
"created_at": "2021-12-22T01:49:32Z",
"updated_at": "2021-12-22T01:49:33Z",
"expires_at": "2022-03-22T01:49:29Z"
}
yeah so
https://github.com/ichard26/black/actions/runs/1612937976 if i go to this without a token
I can't download it
bright glacier
Quite annoying haha
The solution I have in mind is to create a commit in a special repo and then have the web tool (hosted on github pages) download from there as I think downloads from repos work for everyone. Basically copying and pasting how mypyc/mypyc-benchmark-results works but that's a project for later :)
plain atlas
i feel so guilty for working on this project
it uses tabs and single quotes
so i modified black to use tabs and single quotes..
plain atlas
The solution I have in mind is to create a commit in a special repo and then hav...
does everyone need to be able to download?
bright glacier
Well it'd be awesome because then the web tool could also download the artifact
plain atlas
ah
oh btw i may have found an indirect black bug which was made more obvious when changing stuff to format this code
i hope my opinion is still valid
bright glacier
so i modified black to use tabs and single quotes..
you just created another black fork, congrats
plain atlas
;-;
I'm never pushing it anywhere
but since I was refactoring a large portion i wanted to take the oppertunity to format what I can
but tabs should be supported*
*the line length option does not consider the length of a tab for any regard, although IMO there should be either a way to set tab length to be considered for counting the length of a string/line
brb
bright glacier
wait a second, I could just link to this potentially https://github.com/ichard26/black/runs/4611358466?check_suite_focus=true#step:14:1
it's not as clean as viewing a HTML document but you don't have to download or unzip anything
dense jungle
oh yes, that's pretty good
plain atlas
but tabs should be supported\*
\*the line length option does not consider the l...
like, max line length of 120 characters, but there's a tab in the string, which most ides render as 4 characters, so there should be a way to consider a tab as 4 characters for formatting
flat krakenBOT
bright glacier
oh yes, that's pretty good
Actually do we still want to embed a truncated version of the diff in this case? I'm just wondering, it's not hard to implement but I'd rather not implement features we won't use (much).
plain atlas
@bright glacier don't use lightmode plz
bright glacier
.. huh
bright glacier
Updated workflow comment (this time in dark mode because one day I'm going to be @ mentioned because my screenshots are in light mode typically)
bright glacier
Yeah hopefully I didn't tick that poor person off
plain atlas
<@!429445826251587616> don't use lightmode plz
.
bright glacier
oh I did not notice your very late edit
plain atlas
uhuh
bright glacier
https://github.com/ichard26/black/runs/4613437959?check_suite_focus=true#step:14:1 <- this is the diff link now btw
plain atlas
did these actually error?
bright glacier
No, GitHub just highlights anything that matches error: * / warning: * 😦
bright glacier
you do realize this third-party code?
bright glacier
well to be fair the code that's being reformatted (and shown in the diff) is diff-shades itself so I do control it but there's also projects I don't control in the analysis
plain atlas
yeah, the ones that you can't control are likely fine
bright glacier
https://github.com/ichard26/diff-shades/blob/3013dd29001efaa3f5feea47ca4c26431c6a3780/src/diff_shades/config.py <- current list of projects
late dewBOT
src/diff_shades/config.py line 66
Project("scikit-lego", "https://github.com/koaning/scikit-lego"),```It hasn't crashed yet lol
plain atlas
that's why its a nit lolol
bright glacier
Feel free to submit the first outside PR to diff-shades
https://github.com/ichard26/diff-shades/blob/3013dd29001efaa3f5feea47ca4c26431c6a3780/src/diff_shades/config.py#L43-L56 I honestly don't know if these args are necessary nowadays -- I just copied and pasted it from black-primer
late dewBOT
src/diff_shades/config.py lines 43 to 56
Project(
"django",
"https://github.com/django/django.git",
custom_arguments=[
"--skip-string-normalization",
"--extend-exclude",
(
"/((docs|scripts)/|django/forms/models.py"
"|tests/gis_tests/test_spatialrefsys.py"
"|tests/test_runner_apps/tagged/tests_syntax_error.py)"
),
],
python_requires=">=3.8",
),```
bright glacier
LOL amazing
plain atlas
didn't submit that if you were looking forward to it sorry
bright glacier
Now you get to enjoy my high quality CI workflow (tm) /lh
bright glacier
Well the lint job passed so I don't see anything that would fail now
bright glacier
I'm impressed the macos jobs are faster than the windows one
usually file IO on mac sucks so much
bright glacier
bright glacier
We're probably gonna lock the second one under --future but we in general really like the first one but it needs some tweaking
plain atlas
yay!
the whole reason I made a pr obviously
that contributor badge on all future comments on that repo
lets see if i can find a nit on mypyc wheels
bright glacier
whelp.
plain atlas
late dewBOT
test-requirements.txt lines 1 to 4
# This is the bare minimum needed to run the test suite. Why not just use
# `test_requirements.txt`? Well it pulls in stuff we don't need and that
# slows the down testing phase quite a bit.
pytest >= 6.1.1```to run the test suite only pytest is truly needed, everything else in test_requirements.txt is extraneous (but still useful in general)
plain atlas
but there's no test_requirements.txt?
bright glacier
I'm referencing the one in psf/black ...
plain atlas
that is test_requirements.txt
plain atlas
I'm referencing the one in psf/black ...
ahhhh, that was not clear
well you know what's about to happen if you want me to make a pull
bright glacier
one day I'm going to block you for spam /lh /s
actually, go ahead because I just spotted a typo
plain atlas
oh, what typo?
bright glacier
slows the down testing phase quite a bit.
plain atlas
flat krakenBOT
dense jungle
Actually do we still want to embed a truncated version of the diff in this case?...
I think having an easy link to that diff should be fine
bright glacier
https://github.com/ichard26/black/pull/5#issuecomment-999845088 .. how does this feel now?
bright glacier
The diff will quickly overfill the comment length limit so I'd prefer just keeping a link to the logs
tired shard
pastebin link maybe? or something similr
bright glacier
https://github.com/ichard26/black/runs/4613437959?check_suite_focus=true#step:14:1 this link isn't good enough for you?
tired shard
also the link doesn't jump you to the diff, we need to manually click generate html diff
tired shard
it is good enough now, but i would like to see syntax highlight for diffs with minor changes
bright glacier
that breaks the github logs
tired shard
yeah, that's why i was suggesting a pastebin link or something
bright glacier
uhh how am I supposed to get syntax highlighting for both the diff and python format
bright glacier
that breaks the github logs
tired shard
dunno, maybe something like dandavison/delta and export it as HTML
bright glacier
and then where am I supposed to host the HTML? You do realize this will probably add at least 150 lines to the already complex workflow
sorry that's I'm being a little rough I'm ridiculously tired and I probably shouldn't be dealing with this right now tbh
tired shard
can't GHA take html output and show that, the way you generate the html diff
yeah i understand, its fine 👍
plain atlas
also the link doesn't jump you to the diff, we need to manually click generate h...
it does for me
tired shard
which browser?
plain atlas
ff
bright glacier
can't GHA take html output and show that, the way you generate the html diff
the github actions logs aren't in HTML - the step is writing HTML to a file but in the process a diff is printed to the terminal
tired shard
and what do you do the with html diff?
bright glacier
Upload it as an artifact
tired shard
ah well, we can just do one thing if we really want to get that diff then 🤷♂️ write a small web app to get those artifacts and host them for sometime
bright glacier
dunno, maybe something like dandavison/delta and export it as HTML
This seems fine although I'm not sure if I can simply pass a diff to it and have it syntax highlighted by the tool which would make life so much easier
bright glacier
ah well, we can just do one thing if we really want to get that diff then 🤷♂️ ...
Yeah this is more complicated you'd expect though as GH requires a token to access the artifacts.
tired shard
yeah, i had done for a personal project, it basically runs some tests on a user's pr submission (which is like a project) and uplaods the report to our class site
can you share the link to the code snippet which is generating the diff on diff-shades
bright glacier
It's spread over many files but here's the initial bit: https://github.com/ichard26/diff-shades/blob/603f77a9e6805b0c4e4f383c0fc5a5db0140f229/src/diff_shades/cli.py#L77-L99
late dewBOT
src/diff_shades/results.py lines 168 to 178
if "failed" in (r1.type, r2.type):
if not diff_failure:
raise ValueError("Cannot diff failing file results.")
first_dst = f"[{r1.error}: {r1.message}]\n" if r1.type == "failed" else "[no crash]\n"
second_dst = f"[{r2.error}: {r2.message}]\n" if r2.type == "failed" else "[no crash]\n"
else:
first_dst = r1.dst if r1.type == "reformatted" else r1.src
second_dst = r2.dst if r2.type == "reformatted" else r2.src
return unified_diff(first_dst, second_dst, f"a/{file}", f"b/{file}").rstrip()```and unified_diff and color_diff are basically copied and pasted from black's codebase
tired shard
ok cool, I will try something once i get time
bright glacier
please note I won't accept a change that makes it a hard requirement (a soft requirement is fine) to use a third-party application -- also the --no-colors and --force-colors flag still need to work properly
late dewBOT
bright glacier
The main thing I don't like about rich's highlighters is that it usually ends up requiring adding a bunch of terminating spaces which breaks a lot of scripting usage
it's more acceptable for the compare command but it's unacceptable for the show command which is explicitly designed with scripting (and piping) in mind
But I'm just rambling, so if you can come up with something that's native with rich we can probably make it work
tired shard
The main thing I don't like about rich's highlighters is that it usually ends up...
umm could you explain this a bit more
bright glacier
So rich's Syntax feature adds a background which is nice except it needs terminating spaces on each line so the background color code have something to apply on
Now I could just disable the background but then on certain terminals (usually light themed ones) the syntax highlighting is unreadable.
I could use the --no-colors/--force-colors flags as escape hatches but I'd prefer to not have someone's experience degraded by simply using a terminal whose palette which isn't ideal
tired shard
ah right
bright glacier
So rich's Syntax feature adds a background which is nice except it needs termina...
I don't know whether this is always the case but rich doesn't seem particularly open to the idea of avoiding the terminating spaces anyway
tired shard
i will see if i can find a way to avoid that, otherwise we can just open a issue on rich
bright glacier
running python -m rich.syntax on a random python file I have laying around and then redirecting the output to a file nets me this
notice those terminating spaces, they're very annoying
tired shard
yeah
what if we disable the bg and change the syntax highlight theme according to the terminal bg?
bright glacier
The question is how do you query the terminal bg?
bright glacier
what if we disable the bg and change the syntax highlight theme according to the...
I'm fine with adding an option to the compare command which enables python syntax highlighting (on top of the pre-existing diff highlighting) and accepts a value of either "light" or "dark" btw so we don't have to write this code
It should be opt-in though
I still have no idea how you can get two different syntax highlighters applied to a single rich text object but I guess that's what you're aiming for haha
tired shard
basically will try to get text of what the rich.syntax does and then apply the colour diff colours on that
atleast that's what i will try first
bright glacier
if you can get that working then that would be amazing and I wouldn't mind plugging it up to the all of the other flags and options it'd have to interact with myself
you're still welcome to submit a fully integrated PR btw but I just wanted to extend the offer so you don't have to do as much work if you don't want to
tired shard
yeah sure 👍 first let me get those working lol
plain atlas
@bright glacier sleep
tired shard
even you
bright glacier
Yeah I will, my head hurts
plain atlas
11pm
tired shard
11 or 12 ig
bright glacier
Ah we live in the same TZ it seems
plain atlas
you and I do
tired shard
i am way ahead lol
bright glacier
AM? jeez
tired shard
ok what about using terminal colours rather than getting dark/light bg cuz i can't find a way to do it
like make it work for all systems
plain atlas
ahhh India with your half hour off timezone just to be annoying-- I mean be the graveyard shift for call centers handling USA calls at our night
(at least this is what I learned in school)
bright glacier
I'm fine with adding an option to the compare command which enables python synta...
how about this @tired shard
bright glacier
also you aren't working on this at school are you lol?
silent apex
hello hello
plain atlas
also you aren't working on this at school are you lol?
I just set up nearly an entire pydis dev setup
bot/lance/site/metricity/snekbox
bright glacier
I am actually prepared to go to bed so don't expect a response from me anytime soon btw Jason
plain atlas
I am actually prepared to go to bed so don't expect a response from me anytime s...
goodyoushouldsleep
little parrot
i dont think its particularly buggy anymore
muted haven
As far as I can tell, it should work pretty-much alright. You still need to pass -t py310 either as an arg, or in your config though.
muted haven
The latest changes are present in 21.12b0
devout aurora
`src/diff_shades/results.py` lines 168 to 178
```py
if "failed" in (r1.type, r2....
thanks
mint barn
is there some dark web shit going on cuz "black formatter" does not sound legal
bright glacier
I mean, if this was even a little bit related to the dark web I don't think the Python Discord admin team would let us stay here lol
mint barn
I mean, if this was even a little bit related to the dark web I don't think the ...
he rude if he was gonna kick u out
bright glacier
the reason why the tool is named black is to reference the henry ford quote
you can have any color as long as it's black
bright glacier
he rude if he was gonna kick u out
You realize that the admin team is more than just one person right?
mint barn
You realize that the admin team is more than just one person right?
he looks like he came out of the white house
how rich was ford i need to know
bright glacier
Read the wikipedia article because I have no idea either but I'd bet he was very rich
mint barn
im in a plane i would use the internet if i was ford rich
bright glacier
you're certainly using the internet right now to use discord
mint barn
nah im using da wifi
bright glacier
anyway this is starting to get off-topic from the purpose of this channel, which is to discuss the use and development of the python code autoformatter named black
bright glacier
nah im using da wifi
wifi and the internet are separate technologies
mint barn
im confused bye
bright glacier
oh gosh it's here now
it better not break now running in the psf/black environment for the first time
I can't tell you how nervous I am 😄
I think my main concern is how slow it might be -- I really don't want to implement sharding because diff-shades is not designed for it at all right now. I'm sure it's possible to support it but it'll add so much more complexity
little parrot
why's my code in mypy? it like hardly type checks
bright glacier
Oh sorry I just pulled the first mypy_primer comment I could find, if you want me to change it out I can do that.
little parrot
oh no its fine
bright glacier
Interestingly enough primer is now failing LOL
I should probably turn down the verbosity https://github.com/psf/black/runs/4622218052?check_suite_focus=true#step:9:6489 lol
\o/
bright glacier
I can't reproduce the crash primer reports
oh wait I'm black's own configuration .. I should use its configuration lol
and now I'm getting zero changes .. what is going onnnn
elder tusk
why's my code in mypy? it like hardly type checks
it's actually useful to have code that doesn't pass mypy... helps us find cases where people want to do things that mypy currently might not like (as opposed to just catching regressions). if you want to get rid of it, lmk (or open a PR at https://github.com/hauntsaninja/mypy_primer )
late dewBOT
!remind 2d
It has arrived!
bright glacier
it's actually useful to have code that doesn't pass mypy... helps us find cases ...
oh hi! I just wanted to say thank you for writing mypy-primer, I shamelessly stole many .. many ideas from it when writing diff-shades and the integration linked above 
elfin lagoon
hias
plain atlas
@bright glacier https://github.com/psf/black/pull/2725#issuecomment-1000544310
fork black (which you've done) and make the branch with the feature the default branch then make a pr pointing to that
conversely push a new branch to main (if you rename it, it closes any pulls with that branch) the branch main and make it default
bright glacier
What ...?
I've been pushing to main on the fork this whole time
I just made a new branch for the PR
bright glacier
But I thought it still wouldn't run until its on the target repo default branch and not the PR fork
Have you done this before?
tired shard
!remind 1d
late dewBOT
tired shard
richard enhance verbose pr 
lol
plain atlas
But I thought it still wouldn't run until its on the target repo default branch ...
yeah, needs to be on the target repo, but if you make it your own default branch and open a pull on your own fork it'll test the workflow on your side
bright glacier
I already did that, see the review notes links
plain atlas
which at least is a partial test
bright glacier
richard enhance verbose pr <:lemon_cop:827146371466788864> lol
sigh, I will
plain atlas
I already did that, see the review notes links
this just in, richard is schooling the teacher
bright glacier
haha arl, get rekted /lh
in all honesty even that PR body is quite the handful to read (let alone the actual diff) so I can't blame ya
plain atlas
I see it's not co-authored 😔
plain atlas
no, just me, who helped you with the workflow so much :glare: /j
bright glacier
👀
plain atlas
glare and /j do not go together
/j
plain atlas
side note: I suggest adding a portion of the pr body to the commit message
tired shard
arl also you don't review changelog today, i am not reviewing for a month
bright glacier
you solved a really big problem with how the comment workflow was supposed to work lol
plain atlas
arl also you don't review changelog today, i am not reviewing for a month
it's Christmas eve so
bright glacier
side note: I suggest adding a portion of the pr body to the commit message
will do, I'll perform the merge myself then
plain atlas
'sides I don't want it merged until a few other pulls are merged soooo
also that's off topic to black
tired shard
how would they get merged now?
plain atlas
so to be on topic, did you have trouble formatting that pull with black? 👀
bright glacier
'sides I don't want it merged until a few other pulls are merged soooo
who said I have derailed a channel enough?
it's honestly funny how often staff end up derailing pygen
plain atlas
do you remember when we first were here lol
bright glacier
y e s
bright glacier
it was a 🌈 rainbow 🌈 of various topics
plain atlas
of everything except black
bright glacier
yeaa it does sound weird, it's why you'll rarely see me use "black maintainer" or similar, instead I use "maintainer of black"
tired shard
yeah
plain atlas
yeaa it does sound weird, it's why you'll rarely see me use "black maintainer" o...
I use the formatter of black
bright glacier
well that means something totally different :p
plain atlas
also
the server
why is it called blackd?
like... if we're talking about what sounds weird... black maintainer doesn't sound as weird as blackd
tired shard
black deamon?
bright glacier
Yes
plain atlas
well, ok
bright glacier
Man, I don't know what to say about this kind of stuff
tired shard
richard, is there a wy to specify the background colour only in console markup?
plain atlas
do you mean rich?
tired shard
yeah, in rich
plain atlas
are you just asking richard because of his name ;-;
lol I can give you that review rn if you want but it'll probably suck because of my current mood
tired shard
no, cuz it is related to diff shades
your choice
i am anyways going to get horrible nitpicks
bright glacier
I'm not sure but the syntax class has a bg argument or similar?
!d rich.syntax.Syntax
late dewBOT
plain atlas
i am anyways going to get ~~horrible~~ nitpicks
yess although I only do that on my own projects now :) so if I review a lance pull you won't get it
but probably you would on black lolol
bright glacier
You can't do in the wrapped text itself, no, but the parameter should work well enough hopefully
plain atlas
oh yeah I meant to make a bug repro about what I found
although it's not really a bug
it's just a place where a comment says Edge case isn't actually an edge case
and runs often
like on every single quote character
which @bright glacier actually helped me find when I was modifying black to use tabs and single quotes 🥲
don't hate him
bright glacier
I've made Jelle mad enough before doing similar stuff where it won't phase him that I helped you commit a crime
plain atlas
oh good
and it's not a crime IMO
the real crime is contributing to a project that uses tabs and single quotes
so modifying black so I can format my contributions is not a bad thing
tired shard
I'm not sure but the syntax class has a bg argument or similar?
no, not like, i am getting raw syntax highlighted text and then applying the background according to the diff
bright glacier
I'm very confused at what you're doing
plain atlas
I just told you...
oh yeah the reason I wasn't worried about spaces within docstrings is because this program doesn't have very many
sooo
please save me, ichard
tired shard
soo, this is what i have now
now, i want to apply background highlight to x accoding to the diff
bright glacier
why according to the diff?
tired shard
because i am generating the diff right?
so only the different part would be highlighted
end product ^
plain atlas
damn that is fancy
bright glacier
OH you mean the red and green highlights, not the background color
tired shard
yes, sorry for being confusing
I can't find anything on https://rich.readthedocs.io/en/stable/markup.html
bright glacier
I have no idea, the implementation I stole from black simply looks for lines that start with + or - in the diff but that probably won't work as the input will have ANSI color codes embeded
plain atlas
nvm
plain atlas
I have no idea, the implementation I stole from black simply looks for lines tha...
just strip them
bright glacier
but then the fancy python syntax highlighting will disappear -- that's literally that Jason is aiming for
plain atlas
check if the character(s) is either +, -, or ansi
if ansi increase seeker and check again
bright glacier
tired shard
on red would work?
bright glacier
Also Jason do know if rich's python highlighter will behave kindly to a Python diff that has - / + / +++ / --- at the start of the lines?
bright glacier
phew
bright glacier
`on red` would work?
I just discovered this feature right now and I'm on windows so testing it is a pain
my development setup is on ubuntu
sorry
tired shard
Also Jason do know if rich's python highlighter will behave kindly to a Python d...
that won't be rpesent, we have to codes, now we compare them and diff highlight them by comparing the two set of codes
tired shard
https://user-images.githubusercontent.com/52205/86275526-76792100-bba1-11ea-9e78...
just like this
bright glacier
Oh I see
tired shard
but i would need to implement the diff generaor all over again
bright glacier
Yeah I chose the easy way out by stealing black's output functions
hmm I think that diff-shades will be broken on light-themed terminals anyway since there was a bug in one of them I recently fixed
tired shard
not sure how we do it currently, but what i am thinking is, we generate 2d array which basically:
if the code is "ab\ncd", the array is
[["a", "b"], ["c", "d]]
and then we compare each element
bright glacier
by the way, unified_diff is also used to power the line changes feature (i.e. the number of additions/deletions) so any modification to it to support color should be configurable as in the non-colored state it needs to be performant
bright glacier
not sure how we do it currently, but what i am thinking is, we generate 2d array...
It's 11:30 over here once again so I can't really think intensely but sure?
tired shard
not really, lemme see how diflib does it
bright glacier
I'm a bit worried about the speed but it sounds plausible
plain atlas
Yeah I chose the easy way out by stealing black's output functions
it's not stealing when your a contrib to both and it's mut licensed
mit
shh
bright glacier
it's not stealing when your a contrib to both and it's mut licensed
Fair
bright glacier
I prefer fish but sure
plain atlas
ohhh I really thought you were talking about fish the food
plain atlas
you haven't heard of fish the shell?
this confuseind me again I thought you were talking about a shell found in boddies of water
bright glacier
.wiki fish shell
toxic stormBOT
bright glacier
!remind 15H it mayyy be worth doing a security audit of the current workflow setup
late dewBOT