Remove hardcoded color scheme count

[kenmo-pb]

This PR removes the hard-coded Default Color Schemes count in the Preferences.pb DataSection.

Instead, it marks the end of the color schemes with an empty name string (Data$ "") and automatically counts the schemes while it reads them.

This has no effect on the IDE end user. But it has benefits for those working on the IDE project:

• easier to add (or remove) color schemes – no need to remember to manually update that count
• no more git conflicts when people add schemes – for example, mk-soft added "Dark Mode" but that count conflicted with my own branch which adds "Solarized Light" and "Solarized Dark"
• the SpiderBasic and PureBasic counts (two different Data.l lines) can no longer get out-of-sync. :warning: In fact, I think they are out-of-sync in the repo right now. Both counts are hardcoded as 10 even though SpiderBasic adds one extra scheme!

Note: I considered marking the start and end of the color schemes with named Labels, and then calculating the number of schemes like (?EndOfSchemes - ?StartOfSchemes) / BytesPerScheme but this approach doesn't work since the name strings are not a fixed number of bytes.

[kenmo-pb]

I pushed a simpler version, that doesn't use variable i at all. :heavy_check_mark:

Also it fixes a 1-character typo existing in ProjectManagement.pb which prevents SpiderBasic mode from building. (C style |= was used instead of just |) :heavy_check_mark:

However Travis CI here says the build failed, and I have no idea why. It builds locally for me. :x:

[fantaisie-software]

Probably my last commit, it's the The following files

./Residents/Common.pb

which contain trailing whitespace... Can you try to fix it in your commit ?

BTW, all this is a bit annoying tbh, I don't want to create a separate branch everytime just to see if Travis is happy about whitespace and such.

[Naheulf]

I don't want to create a separate branch everytime just to see if Travis is happy about whitespace and such.

The validation script is included in the repo and is Branch agnostic. So you can run it on your side. (See CONTRIBUTING.md#code-styles-validation for setup and usage)

[t-harter]

I think we should get rid of that whitespace check in general. It is annoying even in the feature branches. The majority of submitted PRs have a "fix whitespace" commit in them. I'd rather have some whitespace in the code (which is harmless in PB) than have the commit log littered with such commits.

[tajmone]

I think we should get rid of that whitespace check in general. It is annoying even in the feature branches.

It could be disabled on dev and enforced on master, but it wouldn't be wise. These checks prevent situations like someone editing a PB source with a modern editor which trims all trailing spaces, which would result in every spaces-only being stripped of the indentation that Scintilla puts in empty lines between indented lines. In a source with more than 1000 lines this could mean well over a hundred of noise changes ending up in the commit, making it hard to review PRs. Not to mention that rebase operations would become a nightmare, since these whitespace changes would introduce conflicts resolution even in files where actual code didn't change.

The majority of submitted PRs have a "fix whitespace" commit in them.

That's because the PB IDE is one of vert few editors which doesn't provide trimming trailing spaces, not even when reformatting the source code. These trailing spaces are usually the result of autocompletion, e.g. you type Procedure and then Tab to get EndProcedure, add the procedure name, etc., but when you press ENTER you end up with trailing spaces.

I think it would make more sense to tweak the code reformatting functionality in the PB IDE, to make it trim all trailing spaces — possibly even those in spaces-only lines, which is something you basically get only with Scintilla based editors.

I'd rather have some whitespace in the code (which is harmless in PB) than have the commit log littered with such commits.

It might be harmless for PB, but it can soon become a pain when working with Git. Version control best practices don't just come out of the blue, there are reasons behind them. Going against the grain of GitHub best practices only to avoid facing the problem that the PB IDE doesn't natively offer functionality to trim trailing spaces seems counterproductive to me.

[kenmo-pb]

I am OK with having the validation script. But is it working properly?? I can't find any lines in Residents/Common.pb that have trailing whitespace! Also, this PR did not modify that file...

I agree with @tajmone that dozens or hundreds of formatting changes committed to the repo history is a bigger problem. Ideally, everyone should review their diffs before submitting, and avoid submitting formatting change lines. Any good Git GUI will let you selectively stage or revert specific changes within a file.

Also, minor fixes like whitespace don't need to add extra commits to the repo history. For simple changes (like mine above with variable i) you can just do an "Amend Last Commit". For bigger cleanups, some git rebase -i might be needed. Either way, if a PR fails validation, it is possible to fix without adding correction commits.

[t-harter]

Your fears are way overblown. At work we are maintaining a large codebase with a large team and there is no rule for trailing whitespace whatsoever (only for avoiding tabs). There has never been any of the problems you describe. This is a total non-issue.

I think it is much simpler to catch unintended large whitespace changes in PR review than teaching every new contributor how to deal with the stupid validation script (especially since you cannot even run it on Windows which makes it useless for most people). We have seen this a number of times already. It is just frustrating for all people involved.

If you do insist on editing PB source code with a different IDE, just turn off the whitespace correction and you will be fine. Don't force your own purist views on everybody else!

[tajmone]

Your fears are way overblown. [...] This is a total non-issue.

These are not "my fears", it's what the developers community agree on as being good collaborative standards. There are so many guidelines on this that I don't even have to justify it — and, BTW, GitHub natively supports the EditorConfig standard for rendering code in the WebUI.

I think it is much simpler to catch unintended large whitespace changes in PR review than teaching every new contributor how to deal with the stupid validation script (especially since you cannot even run it on Windows which makes it useless for most people).

How comes the «stupid» Bash script can't run under Windows when Git for Windows ships with Bash? and Win 10 natively supports Bash too — as if Make was natively available on Windows, yet the repository requires it to build.

The point is not teaching contributors how to run a Bash script, it's about asking them to trim any trailing spaces which they add during editing — it's not as if it's asking them to solve Fermat's problem! it's about keeping their code edits clean, leaving behind them code that adheres to the project's convention. In any editor removing trailing spaces is a one-click operation.

But honestly, if you delete the «stupid script» is perfectly fine with me.

[t-harter]

How comes the «stupid» Bash script can't run under Windows when Git for Windows ships with Bash? and Win 10 natively supports Bash too — as if Make was natively available on Windows, yet the repository requires it to build.

You can build the IDE now on Windows without any preconditions in case you haven't noticed. Can't say the same about the validation script:

~~~ ERROR! ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In order to run this script you need to install EClint (Node.js):

        https://www.npmjs.com/package/eclint
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[kenmo-pb]

OK I understand the situation now. Residents/Common.pb got a trailing whitespace when Fred committed it without a PR, so it didn't go through validation. That's why this unrelated PR failed the validation. No big deal.

I deleted the space, amended the commit, force pushed, and it passes Travis CI now. :heavy_check_mark: Please review, it was supposed to be a quick and harmless PR (and a bugfix for SpiderBasic IDE). Unfortunately it turned into a "meta" git discussion again.

We can discuss the validation script in another thread. If @fantaisie-software @t-harter want to disable it, we could try that and see how it goes.

[HeX0R101]

If that whole discussion is about nothing but (annoying) trailing whitespaces, why not simply add a small (could be written in PB in no time) tool, that checks and removes all trailing whitespaces? (maybe also TABs and add missing BOMs) Then add a note in the readme, that contributers should run that for a rough validity clean-up before submitting.

[tajmone]

If that whole discussion is about nothing but (annoying) trailing whitespaces, why not simply add a small (could be written in PB in no time) tool, that checks and removes all trailing whitespaces? (maybe also TABs and add missing BOMs)

I wrote the infamous "stupid script", in the very early days of this repository, as a result of practical problems which we were facing with commits and PRs:

1. Commits/PRs that contained PB settings in the sources.
2. Commits/PRs that didn't add a BOM to PB sources (and, if left that way, could soon lead to encoding corruption, since the PB IDE would then save all files in ASCII, and Git has no way to handle encodings).
3. Commits/PRs with trailing spaces (as mentioned above).

Then add a note in the readme, that contributers should run that for a rough validity clean-up before submitting.

I also wrote the guidelines, quite detailed as a matter of fact.

The infamous script was created as the result of discussion on the repository. Besides the script, the important part is the .editorconfig settings file, which also covers code style convention for other files types, and which GitHub uses to format code and diffs in the WebUI, and which modern editors can automatically pick-up and abide to.

The main purpose of the "stupid script" is to validate PR commits on Travis CI, to warn maintainers about potential code problems with any PR, so they might inspect the problem and fix it, instead of incorporating it into the repository and let these problems pile up.

Being able to run the checks locally is and added benefit, but really only necessary when editing sources with an EditorConfig non-compliant editor (i.e. an old generation editor).

Writing a tool like you mentioned, in PB, would surely help, since validating trailing spaces in PB files is currently done via Bash and SED (or GREP, I don't remember right now) since EClint (the EditorConfig validator) doesn't really support checking for trailing spaces but allowing spaces-only line (which the PB IDE creates, via Scintilla). This app would have to compiled under Ubuntu 64-bit in order to be usable on Travis CI.

At the time I wrote the "stupid script" I settled for the commonly used tools for the job available then (EClint). Writing it from scratch in PB would take time, something we are always in shortage of, especially when getting older. FOSS collaboration is, ultimately about sharing our most precious resource in life: time! We are willing to give up some our time in order to contribute to something we like. Once spent, time can never be recovered, and each one of us as an allotted amount of it in its life.

After the last exchanges in this thread, I'm not quite sure what to make of the collaborative efforts. The code conventions, the scripts and tool used to validate and enforce them, all of that was discussed and agreed upon, and then implemented. Now it turns out that we're facing problems with an excess of PRs that fail the build, mostly because of some commits that went directly into the repo bypassing the PR mechanism and validation (i.e. they are validate only after being pushed). Previous commits failing the build also make all following commits and PR fail it too — I've written a solution to validate only the files changed by a specific PR, but it turns out that it's too slow to be usable, due to the huge number of files in the project.

You also have to consider that presently the repository has a major problem with the PB Help source files, which are encoded in "ASCII" and we've seen various encoding corruption problems already, since Git doesn't care about encodings, and it's impossible to validate single char encodings — so one user might encode a file in Latin1, another one in another encoding (including UTF-8), until the file breaks up (as is already happening).

The current problem is that the repository is allowing commits which are not validate locally, and it's these which are jeopardizing the build. The whole idea of enforcing a code style convention was to prevent this happening, so there's no point blaming the script for the current situation (or Node.js, GitHub, etc.).

What's the point in creating a new dedicated tool when the current problem raises from the fact that some contributors are bypassing the one we have (which, BTW, works fairly fine, and has prevented commits which stripped the BOM from PB sources). One maintainer is not using (and bypassing) the system we agreed to enforce to preserve code consistency, the other maintainer (which was never favourable to the system) sees the current situation as a reason to drop the validation, so everything points in the direction that the repository owners are willing to step back on the code convention question entirely.

IMO, that will be a mistake, and could lead to problems with the PB sources similar to those we're seeing with the DocMaker .txt sources. Also, I remember quite well how frustrating were the first months of the repository, when most commits contained IDE settings in the source, and why we had decided to setup the whole code validation system in the first place (it's all documented in the closed Issues).

@HeX0R101, I admire your proposal to create a dedicate PB tool, but my advice is to spare your (precious) free time for something else. No tool can solve the main problem of the commits which are bypassing the system, so even a perfect tool (not a "stupid" one like mine) will be useless if bypassed by those who have privileges to do so. So Travis CI will still fail the build for clean PRs, because of the malpractices of previous commits, and ultimately the blame would fall on your new "stupid tool".

To make it simple, it's like if we had agreed to install a smoke alarm in the house, but then some of us smoke at home and trigger the alarm, and now we're blaming the alarm because by doing its work is disturbing our peace. You can't have the cake and eat it at the same time.

[t-harter]

For the record: I am not opposed to the validation at all. I just don't see the sense in the whitespace check given that such whitespace has no significance in PB and it is the cause for most failed validations. The hassle with this check simply outweighs the benefit it provides.

You are blowing this way out of proportions IMHO. Please stop acting like this would be the end of the world.

[tajmone]

For the record: I am not opposed to the validation at all.

Sorry for jumping at conclusions then, I seemed to remember a similar discussion regarding Makefiles.

I just don't see the sense in the whitespace check given that such whitespace has no significance in PB and it is the cause for most failed validations

In this case, we could disable just the trailing space validation — which, BTW, is not handled by EClint but the Bash script, due to the spaces-only lines. I don't see the reason to "throw away the baby with the water", since other checks like BOM, encoding, indentation style, etc., are important to preserve the code, especially on other files types.

Also, I'm not quite sure what the problem is, since the failed validation doesn't prevent merging a PR — you can control these settings in the repo, and decide whether failed builds should prevent merging or not (IRC, here they don't). In most of my repos, when I see that the build fails simply because of trailing spaces, I just merge it and then fix or amend the problem in the next commit. But the warning allows me to keep an eye for BOM or encoding problems, which I don't merge until the PR if fixed, since these types of problem can easily propagate into rebase operations, introducing conflicts.

You are blowing this way out of proportions IMHO. Please stop acting like this would be the end of the world.

I don't think it's the end of the world, and I clearly stated that I'm fine with deleting the validation script(s), they are not mandatory. But the scripts are not the problem (if there ever was a problem), since they work fine.

The point I'm trying to make is that we need to enforce more code style consistency, not less, because of the serious problems we're facing with the Help sources due to their ISO encoding and the .txt extension (see #155). Working with single-byte encodings can be problematic today, especially with Git and GitHub, so these conventions and tools can prevent many undesirable problems.

[Naheulf]

You can use this gist as a PB external tool to check if there trailing space in a file. https://gist.github.com/Naheulf/142b261dfc9b5efba043cd8624357803

If you add it as external tool in PB IDE you can use %FILE or %TEMPFILE as argument to auto-select the active file. In this way it will auto check the file and don't show the message requester if the file is OK.

[HeX0R101]

That's a pretty nice start, although not exactly what I had in mind. The past has shown, that those wrong characters are not only in the files the contributers changed, but sometimes also in files which have been changed by those bypassing the syntax check script. I thought about a (console) tool, which scans all PB files for tabs, missing BOMs, PB comments at the end of the files and trailing whitespaces and simply outputs the result in the console. Just to make sure, people don't submit PRs, when they wouldn't pass anway. And of course also to flag their own mistakes

[tajmone]

I thought about a (console) tool, which scans all PB files for tabs, missing BOMs, PB comments at the end of the files and trailing whitespaces and simply outputs the result in the console.

There's editorconfig-checker, which is a dedicated EditorConfig validation tool, written in Go, so it's a standalone binary application:

https://github.com/editorconfig-checker/editorconfig-checker

The reason we're still using EClint (Node.js) is due to a bug in the Windows version which was reporting false positives for line-ends checks (see editorconfig-checker/editorconfig-checker#164). The bug has been fixed a short while ago, but the Node.js package hasn't been yet updated — the NPM package is more practical for Travis CI validation, but for local validation the binary app is just fine.

I've been waiting for almost a year for that bug fix, so that I may stop using EClint and switch to editorconfig-checker. When the NPM version is updated to the bug-fix, or a GitHub action becomes available (most likely, it will use NPM/Node.js though) I'm planning the switch on all my repos — and I'd like to propose it also here, if there's interest in keeping code-style validation (for all files types, not just PB).

This tool handles more code styles checks than you mentioned — e.g. empty end line checks, indentation styles, and encoding validation (except single-char encodings), some of which are non-trivial to implement, and provides some extra features for C styles comments, etc. Replicating all those features might require considerable work and time.

But validation of PB files also requires checking that IDE settings are not stored in the source files, which neither EClint nor editorconfig-checker can handle — which is currently done via Bash script. So that might be worth implementing.

Existing PB Validator Tool

IRC, @SicroAtGit has already written a tool for this, which he uses to validate commits at his PB-CodeArchiv-Rebirth repository, but right now I can't remember where it can be found in the repository. It's definitely worth recycling that part of the code, if you intend to create a validator.

The important checks to be carried out on PB sources are:

1. Presence of UTF-8 BOM.
2. Valid UTF-8 encoding.
3. Absence of IDE settings comments at file end.
4. Spaces indentation (instead of tabs), any number of spaces allowed.

Checking for trailing spaces is a bit tricky, for you'd have to allow for spaces-only lines, which are created by Scintilla when there's an empty line between indented lines.

UTF-8 validation is fairly simple, you can find many sample algorithms online.

[HeX0R101]

Not sure, why I should check UTF8 validity when most contributors use the PB IDE anyway.

[fantaisie-software]

Let's open another topic to see how it can improved. I'm not against it, I'm just frustrated to do things twice right now, as my time dedicated to PB is rather limited these days.

[SicroAtGit]

@tajmone My tools don't do much that would be useful here. Removal of PB settings at the end of the code and syntax check with the PB compiler is quickly programmed. But thanks for the mention.

Let's continue the discussion here: #203