Skip to content

Comments

[Python] Add function: font.isWorthOutputting()#4522

Closed
HinTak wants to merge 8 commits intofontforge:masterfrom
HinTak:issues-3107
Closed

[Python] Add function: font.isWorthOutputting()#4522
HinTak wants to merge 8 commits intofontforge:masterfrom
HinTak:issues-3107

Conversation

@HinTak
Copy link

@HinTak HinTak commented Dec 4, 2020

This is mostly the same as
#3109 , updated for bitrot to work for current master, to address some of the issues raised in #3107

@ctrlcctrlv ctrlcctrlv changed the title Issues 3107 [Python] Add function: font.isWorthOutputting() Dec 13, 2020
@ctrlcctrlv
Copy link
Member

Overall I don't oppose this @HinTak, although another thing I'm wondering about now is why you didn't also add glyph.isWorthOutputting?

@khaledhosny
Copy link
Contributor

There is already glyph.isWorthOutputting() and it has been there since forever, not sure why a font-level one is needed at all.

@ctrlcctrlv
Copy link
Member

Thanks @khaledhosny — I'm not sure why I didn't see it 14 hours ago. I did Ctrl+F through the documentation. Perhaps I accidentally had «Whole Words» checked, I inadvertently do that often. 🤦‍♂️

@ctrlcctrlv
Copy link
Member

@HinTak keeps marking my review comments as resolved despite the fact that they aren't.

So I am not going to keep commenting using that system.

Generally we mark resolved only if we make the change. We allow the commenter to mark resolved if we reply without making a change. This isn't a rule, just etiquette.

I agree with Khaled. This function isn't even really needed. My original comments were made before I knew the glyph level function exists due to browser search issue. One could just do:

any([g.isWorthOutputting() for g in font.glyphs()])

@HinTak
Copy link
Author

HinTak commented Dec 14, 2020

From my point of view, "resolved" meant "addressed". Sometimes, "addressed" means "I don't see any problem with that; no change required". Is that too difficult to understand? The pull is an update for a bit-rotten pull from 3 years ago - to be honest, I don't remember all the details.

Bit-rotten means no-longer merge cleanly; partly it is move to cmake, partly it is that test case is numbered sequentially. When I wrote the test case, it was test case 10; an unrelated test case 10 got merged, so it could not be. So it is now test case 16.

@HinTak
Copy link
Author

HinTak commented Jan 11, 2021

@yg8ijvjvjv please read #3107 and #3085 before you make any further comments. Thanks.

@ctrlcctrlv
Copy link
Member

Any idea why your test case fails on Linux, etc? https://github.com/fontforge/fontforge/pull/4522/checks?check_run_id=1645240966

@frank-trampe
Copy link
Contributor

I was going to try to restart the checks, but I can't figure out how to do it.

@jtanx
Copy link
Contributor

jtanx commented Feb 13, 2021

I think the failure reason is pretty clear, this is not how you write conditionals in Python:

if ( !font.isWorthOutputting() ):

@HinTak
Copy link
Author

HinTak commented Feb 13, 2021

I think the failure reason is pretty clear, this is not how you write conditionals in Python:

if ( !font.isWorthOutputting() ):

That change was made in a rush, as a response to @ctrlcctrlv's request. Yes, it should be "not ...", I guess.

@frank-trampe
Copy link
Contributor

@HinTak, I'm trying to get pull requests merged or closed. Do you still need this functionality? What is the use case?

@jtanx jtanx closed this Jun 27, 2021
@HinTak
Copy link
Author

HinTak commented Jun 27, 2021

@jtanx what do you close this for?

@jtanx
Copy link
Contributor

jtanx commented Jun 27, 2021

No answer to the question above for over two months + questionable usefulness considering existing functionality.

@HinTak
Copy link
Author

HinTak commented Jun 27, 2021

The original pull #3109 was 4 years ago. So it is very slow going, and quite intentionally - @jtanx please leave it open.

@HinTak
Copy link
Author

HinTak commented Jun 27, 2021

@jtanx don't you have better things to do than messing with github on a Sunday?

@jtanx
Copy link
Contributor

jtanx commented Jun 27, 2021

Wow, no need to take things so personally.

Can I please just get an answer to the question posed?

@HinTak
Copy link
Author

HinTak commented Jun 27, 2021

@jtanx as I said, don't you have better things to do than messing with github on a Sunday? I do.

@jtanx
Copy link
Contributor

jtanx commented Jun 27, 2021

Indeed, I have better things to do than reviewing 4 year old PRs with no reasonable explanation provided, so I guess we'll keep this closed then.

@HinTak
Copy link
Author

HinTak commented Jun 27, 2021

@jtanx please leave the issue open.

@HinTak
Copy link
Author

HinTak commented Jun 27, 2021

@jtanx nobody asked you to work on Sunday so please don't. Leave this open.

@skef
Copy link
Contributor

skef commented Jun 27, 2021

@HinTak Beyond the occasional modest grant everyone who works on FontForge is a volunteer. I understand how this particular action annoyed you but the particular times at which people choose to volunteer are not your business.

Just answer the question, or not, as you can on your own schedule and the same thing can happen as if it were being answered today. A few days or a week of delay won't be held for or against the viability of the PR.

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.

6 participants