Skip to content

Conversation

@nigejohnson
Copy link
Contributor

Draft pl/sql coding standards

@ben-sagar
Copy link
Contributor

ben-sagar commented Apr 11, 2019

1.2 The Derivation of these Standards
These standards have been derived from hands-on experience at the Environment Agency over many years

And other parts of Defra too.

Plus, reference to external standards and guidance - this could be useful for people who want to do a bit more reading up.

@ben-sagar
Copy link
Contributor

2.3.2 All tables should have a corresponding error table and all DML statements should be suffixed with a LOG_ERRORS

I think saying all tables is a bit strong. This is really only applicable where some sort of bulk insert/update/delete is being performed.

Also, I think this should be "LOG ERRORS" (i.e. no underscore).

@ben-sagar
Copy link
Contributor

3.3.2 Where you have lists, e.g. a list of column names or values, place each item on its own line, with commas, where needed, placed at the start of the line with a following space

I know this is a thing in the database world, but I really dislike it. The reality is that it only makes a difference (compared to putting the commas at the end) when you add things to the end of the list, which is just something that people have got used to in every other programming language.

I'm sure it was helpful back when we were using line editors in SQL*Plus but these days the cognitive dissonance for people used to seeing code anywhere other than the database is more of an impediment than the supposed benefit of being able to insert items into the list (anywhere except the beginning).

If this were part of some externally-defined industry standard that everyone used then I'd probably just shrug and accept it, but if we're defining our own then I'm prepared to do a few minutes of grumbling.

Also, I'd be interested to see if there's some common alignment with the T/SQL standards.

@nigejohnson
Copy link
Contributor Author

Just pushed a new version containing changes in response to the comments on sections 1.2 and 2.3.2.
Still awaiting some further (most likely offline) debate re 3.3.2

@ben-sagar
Copy link
Contributor

I'm comfortable with this going in as a first pass, but I'd want it in Markdown so won't approve a merge until we can do that.

they are mature, comprehensive while also being reasonably lightweight
and follow standard industry practice.

## Important Recommendation: Refer to these Standards by Version as well as Name.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than having this here, I think it should be covered on the standards front page as it applies to all the standards.

I'd welcome some input from @Cruikshanks

Copy link
Member

@Cruikshanks Cruikshanks Jun 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, if this what we're saying when it comes to standards.

So as a first pass should we be saying this document should have a version number (in the title, as a section heading ??) Then these section about rationale, derivation and versioning be moved to standards/README.md.


For example:

> CREATE OR REPLACE PROCEDURE my\_proc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of this (and the other bits of code in here) should be formatted - I would suggest using code pre-formatting:

CREATE OR REPLACE PROCEDURE my_proc
IS
BEGIN
   <<Block1>>
DECLARE
   ......

etc.

Copy link
Member

@Cruikshanks Cruikshanks Jun 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can actually go beyond just code formatting.

CREATE OR REPLACE PROCEDURE my_proc
IS
BEGIN
   <<Block1>>
DECLARE
   ......

GItHub uses linguist to provide syntax highlighting so anything listed there can be specified on your code block.

N.B. Though it appears this code block doesn't give the best example of the syntax highlighting 😁


WHERE histab.col1 = hertab.col2;

# APPENDIX: An Example of Automatically Formatting PL/SQL Code Using TOAD (the “Tool for Oracle Application Developers”)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd split this section out into a separate guidance document in order to fit with the structure of the repo. I expect if we ask nicely @Cruikshanks would do this as part of rebasing to fit the document into the structure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to do this. Would you like me to do this before hand so you can see what it would look like before it gets merged @nigejohnson ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to do this. Would you like me to do this before hand so you can see what it would look like before it gets merged @nigejohnson ?

OK Alan, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reformatted the code blocks in the markdown now.
But I'm not clear what the plan is for making the "versioning" bit more general or splitting out the appendix into "guidance"? (I think those were the two outstanding things).

Johnson and others added 8 commits July 4, 2019 15:09
This was an older PR initially created before the project had any structure.

With this change we've performed a rebase so the new structure appears, and first step is to move the suggested standard into it.
@Cruikshanks Cruikshanks force-pushed the plsql-coding-standards branch from bbe2ead to f6f59ef Compare January 3, 2020 16:46
Renaming to follow the current convention of always using lowercase, and separating names using underscore.
Cruikshanks added a commit that referenced this pull request Jan 3, 2020
This stems from an agreed refactoring of PR #8 (Plsql coding standards). It was highlighted in that PR some of the content is applicable generally, not just to PL/SQL.

So it was suggested to move the more general content into the standards README.

So the content of this PR comes from @nigejohnson and questions and comments should be directed to him 😁

Anything regards structure and how the changes were made, feel free to fire at me @Cruikshanks 😰
Cruikshanks added a commit that referenced this pull request Jan 3, 2020
This is taken from a 'Rationale' section in the PL/SQL PR #8. We have moved it here because of the suggestion it applies to all standards.
Cruikshanks added a commit that referenced this pull request Jan 3, 2020
Include in PR #8 was the recommendation that all standards should be versioned, and where they are used the version should be referred to.

Again this would be some that applicable to all standards, not just PL/SQL so it has been migrated here to allow PR #8 to progress, and this suggestion be discussed in isolation.

For reference, as part of the copy across I have made the following changes

- slight grammatical tweaks to fit the new context
- markdown formatting tweaks, namely using bold to highlight the examples
- removing the line breaks within sentances. There is no need to break lines in markdown at 80 chars, particularly when it is solely for display in GitHub
As discussed in the PR, this section felt applicable to standards generally, not just this one.

Rather than muddy this PR, it has been deleted here, but added as part of a new PR (see #25)
Same as the previous change.

As discussed in the PR, this suggestion would be applicable to all standards generally, not just this one.

Rather than muddy this PR, it has been deleted here, but added as part of a new PR (see #25)
To be honest, I'm not sure of where we got to regards discussion in the PR on this section.

It was certainly highlighted by @bensagar-ea that it could be expanded to include Defra and its other parts, plus external standards and guidance.

This could be ready as a generally applicable to all standards, but then the specifics of each standard might be very different.

One might be 100% a external standard, another might be based on one with our own changes, or 100% of our own making.

So knowing that this section if it was to be retained would need more work, but concious the goal is to get this PR done and dusted I've just gone with deleting it for now.

I've based this on the convention the other standards have followed, which is just to talk about the conventions and rules without any pre-amble. Secondly the project README also contains some general statements similar to this, plus we go into more detail in our [principles](https://github.com/DEFRA/software-development-standards/blob/master/principles/README.md).
@Cruikshanks Cruikshanks added the enhancement New feature or request label Jan 5, 2020
@Cruikshanks
Copy link
Member

Think I have done what was agreed @bensagar-ea . Remember to take into account PR's #25 and #26 if you are wondering where things have been moved to.

Hopefully, this means it can be agreed and merged in your next standards meeting (which I won't be able to attend 😢 )

Cruikshanks added a commit that referenced this pull request Jan 8, 2020
This stems from an agreed refactoring of PR #8 (PL/SQL coding standards). It was highlighted in that PR some of the content would be more appropriate as guidance, rather than being a standard we must all follow.

So it was suggested to extract and move it into the `/guidance` section of the project.

SO the content of this PR comes from @nigejohnson, structure and how the changes were made, was me @Cruikshanks

I had to do a certain amount of editing to the format and content of the pre-amble as part of this.

The original PR also brought in the idea of all standards needing to be versioned, and these versions having to be referred to, which is something we've also extracted out as a new PR (see #25).

As that has yet to be decided I've omitted that content. Also the original contribution marked the content up as headings. That's where I have re-formatted things so we have actual headings, and the extracted text then forms the section content.
I could have sworn I changed this section in a previous commit hence the comment

> To ensure the section on code formatting fits in with the other changes made, and follows what conventions we have (looking at you C# standard!) I've revised the section and added it as the opening preamble to the `Layout section`.

But clearly I didn't, so doing it now.
These appear to have come back in due to a merge of the original branch back into the branch.

`plSqlStandards.md` has been replaced by `/standards/plsql_coding_standards.md`, and the `.opt` file is now part of the guidance.
Now the guidance has been merged we can link to them. So have updated this section to include them.
Cruikshanks added a commit that referenced this pull request Jan 8, 2020
This is taken from a 'Rationale' section in the PL/SQL PR #8. We have moved it here because of the suggestion it applies to all standards.
Cruikshanks added a commit that referenced this pull request Jan 8, 2020
Include in PR #8 was the recommendation that all standards should be versioned, and where they are used the version should be referred to.

Again this would be some that applicable to all standards, not just PL/SQL so it has been migrated here to allow PR #8 to progress, and this suggestion be discussed in isolation.

For reference, as part of the copy across I have made the following changes

- slight grammatical tweaks to fit the new context
- markdown formatting tweaks, namely using bold to highlight the examples
- removing the line breaks within sentances. There is no need to break lines in markdown at 80 chars, particularly when it is solely for display in GitHub
@nigejohnson
Copy link
Contributor Author

Couple of things:
On the "do we refer to standards we have created by an explicit version number or date" we ought to go for an explicit version number. "Date" will get too wordy and confusing when trying to refer to a particular version of the standards, e.g. in project documentation, especially as git strongly encourages lots of branching and frequent commits. Also this is the normal practice for any formal product, for example software products usually have a version number in their own right, clearly "stamped" somehow on the distributed product. As a product can exist independently of its source control system the version of a product should be part and parcel of a product and not dependent on that source control system.
Secondly, should we be entirely losing the sections on the "derivation" of standards? Knowing how a standard came to be and to what extent it is proven by practice will be useful for understanding when and how a standard should be revised. For example, if a standard has been used a lot in circumstances that are still largely unchanged then the standard is likely to still be "sound", but if the circumstances that "evolved" a standard are now known to have changed dramatically, then the relevance of standard in its current form is more open to question. (We are creatures in time and knowing the history of any complex human product is normally essential to understanding why and how it can be changed!).

@nigejohnson
Copy link
Contributor Author

Another random thought triggered by the previous thought: a version control system is the most appropriate place to store content (especially while it is still living content that is subject to change), but is it the best place or mechanism to publish/distribute content?

@ben-sagar
Copy link
Contributor

@nigejohnson A valid discussion to be having on the version numbering, but that's been split out into a separate PR #25 so maybe put comments on there?

@ben-sagar
Copy link
Contributor

@nigejohnson I'm happy to have some background/derivation as part of the rationale behind a standard.

@ben-sagar
Copy link
Contributor

@nigejohnson @Cruikshanks I hadn't realised that the rationale/derivation section had been removed - happy for it to be put back in.

From the feedback in the PR it looks like I got the wrong end of the stick when it came to the derivation section. It was fine to stay in.

So this change brings it back. The one tweak (apart from removiung line breaks) I have made is to drop the section title and just have it as the opening paragraph to the standard.
ben-sagar
ben-sagar previously approved these changes Jan 8, 2020
Copy link
Contributor

@ben-sagar ben-sagar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@ben-sagar ben-sagar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still good, even without the tabs. 😜

@Cruikshanks Cruikshanks merged commit 8d84abf into master Jan 8, 2020
@Cruikshanks Cruikshanks deleted the plsql-coding-standards branch January 8, 2020 14:39
Cruikshanks added a commit that referenced this pull request Feb 10, 2020
This is taken from a 'Rationale' section in the PL/SQL PR #8. We have moved it here because of the suggestion it applies to all standards.
Cruikshanks added a commit that referenced this pull request Feb 10, 2020
Include in PR #8 was the recommendation that all standards should be versioned, and where they are used the version should be referred to.

Again this would be some that applicable to all standards, not just PL/SQL so it has been migrated here to allow PR #8 to progress, and this suggestion be discussed in isolation.

For reference, as part of the copy across I have made the following changes

- slight grammatical tweaks to fit the new context
- markdown formatting tweaks, namely using bold to highlight the examples
- removing the line breaks within sentances. There is no need to break lines in markdown at 80 chars, particularly when it is solely for display in GitHub
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants