HinTak / Font-Validator

Font Validator is a tool for testing fonts prior to release. This testing ensures that fonts meet Microsoft's high quality standards and perform exceptionally well on Microsoft's platform.
Other
146 stars 12 forks source link

further feedbacks from Paul Wise #14

Open HinTak opened 8 years ago

HinTak commented 8 years ago

Many of the further feedbacks from @pabs3 via e-mail are specific to my repo, so I am posting them here. In general, issues applicable to upstream should be filed upstream, like the original thread ( https://github.com/Microsoft/Font-Validator/issues/16 ).

Please include years in your Copyright lines.

When you make commits, *always* write a detailed explanation of why you
are making the change.

When you make commits, always do one logical change per commit, don't
mix up multiple different changes into one commit.

Especially don't make whitespace and other changes in the same commit.

Please come up with a proper version scheme (like SemVer).

http://semver.org/

Tags should be named font-validator-1.2.3.4 (name-version).

Don't copy in code that is maintained elsewhere.

Don't commit binary or generated files to git, including PDF, CHM etc.
Instead commit the source code for them and the autobuild instructions.
For example the ReadMeFirst_MacOSX.pdf has been produced from LaTeX.
Your version of Font-Validator cannot be added to Debian due to this.

Don't commit patches to git, instead send them upstream.

Your distribution of FreeType without distributing source code is a
license violation, please do not do that. I strongly recommend
rewriting your git history to drop all commits to bin/ and the freetype
patch too.

Having two build systems in one project is a bad idea, I would suggest
dropping one of them and keeping the one that is cross-platform.

Why not Compat.2nd and not put things in Compat?

The -quiet option still prints filenames before printing warnings.

When parts of the code have been automatically derived from specs, put
that process in the Makefile so it can be repeated. I would also remove
it from the git repo and make running it a part of the release process.
Otherwise the code is guaranteed to get out of date with the spec.

When pointing out bugs in other tools like Xamarin in the commit log,
give a direct link to the bug report you are talking about.

Many of your commits have amazingly long changelog entries, this is a
sign that they should be split up into smaller commits.

An option to put the reports in the current dir might be useful.

Never ever do stuff like Compat/Xsl.cs, that is ridiculous.

Why do you embed signatures of Windows certs in the code??

What is README-hybrid.txt and why does it have commits in it??

If the project is going to grow more binaries with generic names, they
need to have a prefix on them otherwise they could conflict with names
used by other projects at some point in time.

I'm not sure the repo is the best place to be storing slides for
presentations, I would put those elsewhere.

Cut-and-paste typos are a sign that code needs to be refactored.

base64 encoded binary files inside text files is horrible, is there any
way for resx files to instead reference external binary files?

Please sign your tags and pushes with OpenPGP:

http://mikegerwitz.com/papers/git-horror-story
https://help.riseup.net/en/security/message-security/openpgp/best-practices

Please read Debian's upstream guide:

https://wiki.debian.org/UpstreamGuide

here is part of my response - I see there is an item about write access/CLA in response to a large paragraph from Paul, which I did not include above:

- I don't think Microsoft will ever allow non-employees write access to upstream, even if they can get rid of the CLA (which is also unlikely). For my part, I won't allow write access to mine either - what happens in my repo I am responsible for, and I will review every change that goes in. But I will consider pulls, or cheery-picks. I was not prepared to take whole-sale cosmetic changes ( https://github.com/HinTak/Font-Validator/pull/1#issuecomment-156547330 ) not because I don't agree, but because it takes time to review that they are truly cosmetic.

- when I first put my branch out there, it was organized to cherry-pick implemented features from - i.e. inter-dependent commits collapsed into one. I was not expecting people to adopt it whole-sale; and to some extent, this is still the case: you want some features, there is a minimal number of commits you can cherry-pick from mine (and maybe implement some other features different from how I did it).

- version-wise, I intend to bump up to 2.0 when rasterization is done. Current is 1.2.x internally, and externally, just successive dated snapshots.

- the pdf has a lot of figures... not sure if it is a good idea to splatter the repo with jpgs. I'll figure something out later.

- Freetype (is not GPL - it is BSD-like) allows modified binary distribution without source. Apple does that - freetype is part of core graphics. I do include patches applied and describe the patches in the README. The others in bin are all LGPL - and even GPL does not require *me* to provide source, just telling people where one can get source, and I do. The binaries are all verbatim from fedora binaries (except freetype and sharpfont), and you can get sources from fedora. IANAL though.

- the included patches were rejected by upstream (some were accepted by upstream and already there) as is. It needs further work to be included - discussion elsewhere.

- mono/c# is what I use, but visual studio is more popular. I am allowing the latter hoping to attract contributors.

-  Compat.2nd needs to be built at the 4th (?) stage after some other things than Compat . In general, Compat* are my replacements to Microsoft not-opened things.

-  Compat/Xsl.cs happens, to allow Mac OS X native exe (where one embeds the .net/mono exe with a mono runtime) to work. I might change how it is done later.

- Windows certs : because it needs a minimal number of 'trusted origins' to work, for the common signed fonts. I don't think I can embed the certificates themselves, so I just put enough to identify known-trusted ones.
HinTak commented 8 years ago

README-hybrid.txt describes my private branch, where missing not-opened functionalities are provided by the binary-only dlls from the 2003 binary. I maintain such a private branch so that I have a base to compare with new re-implementations of missing functionalities with. I do not want to encourage development in that direction, but do want to provide enough information to determined individuals to replicate that work, or shows the private branch's existence, if somebody wants to ask questions in that direction, if they must.

The file is information for people who prefers more-functionality-now (using some binary-only components), vs open-source purists (everything in source form, many parts don't currently work, and needed to be re-implemented later).

anthrotype commented 8 years ago

I count myself among those who "prefer more-functionality-now" ;) I understand that you "do not want to encourage development" in the direction of the "hybrid" FontVal. However, it'd be nice if the "README-hybrid.txt" was more than purely informational, but actually provided the patches required to build the hybrid branch from source.

HinTak commented 8 years ago

@anthrotype : Well, if you want to see the hybrid branch, you just need to ask. Here it is, just cherry-pick'ed and pushed:

https://github.com/HinTak/Font-Validator/tree/hybrid.205-05-06

It is definitely windows-only (i.e. does not work with mono); other than that, not much to say about it...

HinTak commented 8 years ago

Oh, I missed a "1". need to rename the branch!

anthrotype commented 8 years ago

thanks! you can just call the branch "hybrid", with no dates.

HinTak commented 8 years ago

renamed to:

https://github.com/HinTak/Font-Validator/tree/hybrid.2016-05-06

as it should be... (2nd try - still thinking it is 2015 sometimes!).

HinTak commented 8 years ago

Since it is just a set of small patches on top of current HEAD, I think it should have a date - it is not really a dev branch in that I am not adding anything on top. I am not doing any pull/merge on top of it. The next time I need to do a hybrid build, I'll just fork off a new HEAD and cherry-pick the same set of patches on top, and make a new "branch".

There is really only one reason why it exists now - the rasterization test.

HinTak commented 8 years ago

BTW, when I said "windows-only", I meant it - the microsoft rasterer backend is a mixed-mode assembly (it is part native windows, part dotnet), and does not work under mono. If you really want to play with it on non-windows, you will need to use wine + genuine microsoft dotnet . i.e. you will need to remove the default wine's mono's based emulation of dotnet, install genuine microsoft dotnet under wine, to get it to work.

Obviously your life will be simpler with running the hybrid branch binary if you are on genuine windows...

davelab6 commented 8 years ago

Your distribution of FreeType without distributing source code is a license violation, please do not do that. I strongly recommend rewriting your git history to drop all commits to bin/ and the freetype patch too.

I doubt it; the FreeType License permits non-source distribution if the copyright notice is here

davelab6 commented 8 years ago

Ah I see you answered this already :)