Skip to content
This repository has been archived by the owner on Nov 17, 2022. It is now read-only.

Add class to represent Standard Versions #273

Merged
merged 34 commits into from
Mar 20, 2018
Merged

Add class to represent Standard Versions #273

merged 34 commits into from
Mar 20, 2018

Conversation

hayfield
Copy link
Contributor

@hayfield hayfield commented Feb 1, 2018

Fixes #265

This adds a class to represent Standard Versions.

It handles both existing and proposed Version Number formats. It also acts as a working demonstration of how the proposed SemVer change could work in practice.

At the moment this has a class that can be created when given a supported version of the IATI Standard, otherwise it will raise an error.
…ion number

At the moment this requires a positive integer, followed by a decimal, followed by a 0, followed by a number from 1-9
The Decimal restrictions are to make the difference between IATIver and SemVer possible. This could be worked around by always zero-padding.
This needs to be determined.
The idea of what a valid IATIver Version Number has been kinda specified. This updates tests to meet this.
This makes the code easier to read.
There needs to be some level of distinction between IATI Versions, SemVer and IATIver.
This adds some level of differentiation.
pyIATI should be SemVer compatible as well as IATIver compatible.
This means the information is now stored
This provides access to a SemVer feature that IATIver does not have.
Design at #265 (comment)

IATIver Versions are assumed to have a SemVer Patch Component of 0.
The parent class has functionality to parse version numbers and ensure that all expected attributes are set.
This commit ensures that this is used.
Also fixes a linting error that stated super() must be called.
str() outputs IATIver strings since this makes most sense for print() to show.
repr() outputs full SemVer information so that it is less lossy.

Some refactoring has been undertaken to make the overall test file more DRY. Some of the helper functions may wish to become part of the main version module.
Some situations require IATIver strings. Some require SemVer strings.
By making it explicit which one is required, potential changes to str() will not impact the code.

The current status of them being properties with particular names may change to them being functions with different names.
The base class returns a non-IATI version. This needs overriding so that an instance of the correct type is returned.
This functionality is not currently required.
An AttributeError is raised since this is what occurs if the attribute never existed. The message this provides is slightly different to if it never existed.
This means that if the underlying library for Versions changes, it'll be easier to locate tests that relate specifically to this implementation.
The utilised base class contains a number of attributes that are required for it to function.
These are not required for IATI, though can be useful in some manner. At the moment, it is being checked that they exist.
The function to check whether a value is in an IATI format is useful outside init.
It has been extracted to both make this functionality usable elsewhere, and to improve readability.
It should be possible to update the Major, Minor and Patch components after initialisation.
This also impacts the Integer and Decimal properties.
Several whitespace changes.
Some pylint errors have been ignored since they are working as intended.
Treating people like adults is Pythonic. It does, however, mean that errors will show up away from where actual errors would occur.
@hayfield hayfield added incomplete A PR that is in a state that is not ready for review. missing-feature A major feature that should exist, but does not. standard-support Relating to how pyIATI supports a major component within the IATI Standard. labels Feb 1, 2018
@hayfield
Copy link
Contributor Author

hayfield commented Feb 1, 2018

Have labelled as incomplete until it's been used fully and its interface tested within #223

Version has to be imported before Codelists due to import order meaning Codelists depends on Version.
At V1, version numbers are Decimals.
This class will contain content other than the Version class.
As per https://softwareengineering.stackexchange.com/a/75929 this means that the module name should be singular.

This allows the module name to exist within the API without sounding silly.
@hayfield hayfield added complete A PR that is in a state that is ready for review. and removed incomplete A PR that is in a state that is not ready for review. labels Feb 27, 2018
@hayfield hayfield requested a review from a team February 27, 2018 09:38
@hayfield
Copy link
Contributor Author

Marking as complete to obtain a review on this as it stands. Other changes can occur at a later point.

The purpose of the module has changed since the docstring was written.
@hayfield hayfield added the versions Relating to IATI Version Numbers. label Feb 28, 2018
Copy link
Contributor

@dalepotter dalepotter left a comment

Choose a reason for hiding this comment

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

Looks good from what I can see so far (here and having got it running locally). No comments over than those raised in #280 )

dalepotter
dalepotter previously approved these changes Mar 19, 2018
Copy link
Contributor

@dalepotter dalepotter left a comment

Choose a reason for hiding this comment

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

Test is definitely quite rigorous!

@@ -21,6 +21,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) a
- [Validation] `full_validation()` now checks whether a Dataset is IATI XML. [#239]
- [Validation] Test that SSOT organisation test files are valid IATI XML. [#242]

- [Versions] Add a class to represent Standard Versions. Handles current and proposed formats. [#273]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest proposed (SemVer) formats.

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'd propose leaving this out, since if you're detailing SemVer then it also stands that current formats (IATIVer, Decimal) should also be enumerated, and that feels a bit messy since IATIVer isn't a commonly understood term.

iati/version.py Outdated

@property
def decimal(self):
"""int: The IATIver Decimal Component of the Version."""
Copy link
Contributor

Choose a reason for hiding this comment

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

For clarity - suggest Adds 1 to SemVer version as IATIVer numbers (within an integer/major) start with .01

Copy link
Contributor Author

@hayfield hayfield Mar 19, 2018

Choose a reason for hiding this comment

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


return is_semver_format and is_permitted_value

def next_major(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have use cases for the creation of next_minor|major ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Primary reason they exist: The underlying SemVer library has them.

They are not currently used within pyIATI.

Copy link
Contributor Author

@hayfield hayfield Mar 19, 2018

Choose a reason for hiding this comment

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

See 7847309 for the relevant commit.

The previous docstring didn't really note what the difference between IATIVer Decimal components and SemVer Minor components are. This adds the info.
hayfield and others added 2 commits March 19, 2018 14:24
pydocstyle is confusing in the things it requires a blank line after, and those that it doesn't D:
Fixes blank line contains whitespace linting error
@hayfield hayfield merged commit 4a2c28d into dev Mar 20, 2018
@hayfield hayfield deleted the add-version-class branch March 20, 2018 14:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
complete A PR that is in a state that is ready for review. missing-feature A major feature that should exist, but does not. standard-support Relating to how pyIATI supports a major component within the IATI Standard. versions Relating to IATI Version Numbers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants