Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ascii integrity checker #1718

Merged
merged 3 commits into from
Aug 11, 2016
Merged

Ascii integrity checker #1718

merged 3 commits into from
Aug 11, 2016

Conversation

stefan-kolb
Copy link
Member

@stefan-kolb stefan-kolb commented Aug 11, 2016

To avoid problems using BibTeX and non-ASCII chars.

Quick tests show that it also marks é as an error. Does anyone know what chars BibTeX can really handle? Umlauts are a problem but é should work?! So i cannot include all of the extended ascii table http://www.theasciicode.com.ar/extended-ascii-code/letter-e-acute-accent-e-acute-lowercase-ascii-code-130.html

@stefan-kolb stefan-kolb added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 11, 2016
@tobiasdiez
Copy link
Member

LGTM 👍 but \usepackage[utf8]{inputenc} also saves the day 😄

@oscargus
Copy link
Contributor

oscargus commented Aug 11, 2016

It's not just umlauts. Anything outside of 7-bit ASCII will typically break LaTeX unless one uses inputenc. Break as in quite likely produce the wrong character in the document.

"Determines whether a character is ASCII, meaning that its code point is less than 128."

@stefan-kolb
Copy link
Member Author

So this checker seems reasonable for you?!

@oscargus
Copy link
Contributor

Yes, without more insight on the exact details (that LaTeX may use some 8-bit mapping that makes sense sometimes, some people may always use inputenc, etc) I would say that this is a good start. Then, ideally I would like the integrity checks to be disabled (or at least the filter settings remembered), but I'm not going to ask you to implement that now. :-)

@oscargus
Copy link
Contributor

(But please merge another PR with translation strings first, so you change your standpoint on union merge... ;-) I do not know how many times I've rebased in the last week where I had to do a manual edit that could be solve with union merge of the translation files...)

@stefan-kolb
Copy link
Member Author

I'm not 100 % sure what was the reason to change union merge but this messed up our changelogs and translations as far as i can remember, adding duplicates and so on?!

@stefan-kolb stefan-kolb merged commit 33b5da2 into master Aug 11, 2016
@stefan-kolb stefan-kolb deleted the ascii-checker branch August 11, 2016 15:34
@Siedlerchr
Copy link
Member

@stefan-kolb Locally it works, but there seemed to be problems with github not supporting this:
isaacs/github/issues/487

@tobiasdiez
Copy link
Member

@Siedlerchr @stefan-kolb so when union merge is enabled, then github don't automatically resolves the merge conflicts and a manual rebase / push is needed, right? (which is still better than the current system since the local rebase would be easier with union merge).
But these problems with github dosn't explain why union merge was actually disabled for the changelog ac08efb

@Siedlerchr
Copy link
Member

@tobiasdiez Yeah. I think we should enable it back again. For the changelog and the lang files.
Conflicting lang files are hell!

@oscargus
Copy link
Contributor

Yes, GitHub did not support it (no idea how it is now), so that wasn't the problem. It did indeed lead to duplicates, but since the alternative is conflict markers and in quite a few cases it completely avoids conflict markers I still do not see the problem with duplicates. I rather remove duplicates half of the time than conflict markers twice as often.

As I recall it some developers got annoyed with the duplicates and removed the union merge.

@oscargus
Copy link
Contributor

oscargus commented Aug 11, 2016 via email

@stefan-kolb
Copy link
Member Author

I though about this too when looking at the current implementation. We can just extract them all in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants