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

homePage fix, StatementRef UUID fix, Identified Group table fix #323

Merged
merged 2 commits into from
May 10, 2013

Conversation

andyjohnson
Copy link
Contributor

Went back and forth, but all changes approved.

andyjohnson added a commit that referenced this pull request May 10, 2013
homePage fix, StatementRef UUID fix, Identified Group table fix
@andyjohnson andyjohnson merged commit 10a4e5d into adlnet:master May 10, 2013
@ZackPierce
Copy link

My apologies if this question was already answered in the recent group call I was unable to attend, but what are the xAPI specification version changes associated with this alteration?

I assume that account objects under version 1.0.0 must have "homepage" properties with a lowercase "p".
What is the new semantic version beyond which account objects may possess "homePage" properties with an uppercase "P"? 1.0.1? 1.1.0? 2.0.0?

Since JSON properties are case sensitive, the statement JSON definition is a key part of the specification API, the change is backwards-incompatible, I think that Semantic Versioning strictly interpreted would require a new major revision, e.g. 2.0.0.

Such strictness may not be socially or politically appropriate, though.

@fugu13
Copy link
Contributor

fugu13 commented May 11, 2013

Hi Zack,

Since the content being replaced was inadvertently added and caught before
long, 1.0.0's behavior is being retroactively defined as containing only
homePage with a capital P.

Russell

Sent from my iPad

On May 11, 2013, at 8:47 AM, Zack Pierce notifications@github.com wrote:

My apologies if this question was already answered in the recent group call
I was unable to attend, but what are the xAPI specification version changes
associated with this alteration?

I assume that account objects under version 1.0.0 must have "homepage"
properties with a lowercase "p".
What is the new semantic version beyond which account objects may possess
"homePage" properties with an uppercase "P"? 1.0.1? 1.1.0? 2.0.0?

Since JSON properties are case sensitive, the statement JSON definition is
a key part of the specification API, the change is backwards-incompatible,
I think that Semantic Versioning strictly interpreted would require a new
major revision, e.g. 2.0.0.

Such strictness may not be socially or politically appropriate, though.


Reply to this email directly or view it on
GitHubhttps://github.com//pull/323#issuecomment-17762159
.

@fugu13
Copy link
Contributor

fugu13 commented May 11, 2013

Perhaps a better way to think about it is we've adjusted the release date,
effectively.

Sent from my iPad

On May 11, 2013, at 8:47 AM, Zack Pierce notifications@github.com wrote:

My apologies if this question was already answered in the recent group call
I was unable to attend, but what are the xAPI specification version changes
associated with this alteration?

I assume that account objects under version 1.0.0 must have "homepage"
properties with a lowercase "p".
What is the new semantic version beyond which account objects may possess
"homePage" properties with an uppercase "P"? 1.0.1? 1.1.0? 2.0.0?

Since JSON properties are case sensitive, the statement JSON definition is
a key part of the specification API, the change is backwards-incompatible,
I think that Semantic Versioning strictly interpreted would require a new
major revision, e.g. 2.0.0.

Such strictness may not be socially or politically appropriate, though.


Reply to this email directly or view it on
GitHubhttps://github.com//pull/323#issuecomment-17762159
.

@garemoko
Copy link
Contributor

It was always homePage in 1.0.0 but an unfortunate misplaced correction led to it being homepage in one of the 9 occurrences in the spec document itself. This error obviously is a source of confusion so it's being tidied up. homepage with a lower case p is invalid for all versions.

@garemoko
Copy link
Contributor

In case anybody's wondering, here's where the error crept in back at the end of March:

46309da#xapi-md-P134

We should have picked it up but sometimes its difficult to spot changes when they come with a big re-structure.

@ZackPierce
Copy link

Righto, thanks for the clarification.

I have updated the validator to account for this correction.

@dnjohnson
Copy link
Contributor

@ZackPierce Just to clarify, SemVer is all about conveying meaning between versions and this point overrides any rule on version numbering. The audience and amount of people impacted can affect the version level increased, so if there were millions of running xAPI applications relying on the property's case then yes, a major level change to 2.0.0 may be necessary since so many applications would break.

On the other hand, since this was caught so early after the release and adopters are small, a patch level release is more appropriate since it's bringing the spec back to it's "intended meaning". I do think the patch level should be changed to convey that there's been an implementation revision (at the moment, someone who's download ADL's 1.0.0 PDF will have no idea that this property has changed whereas a patch level will at least let them know something changed) but I'm not going to get hung up about that.

For more information, SemVer covers a similar situation in their FAQ - "What should I do if the bug that is being fixed returns the code to being compliant with the public API?"

@fugu13
Copy link
Contributor

fugu13 commented May 16, 2013

I have to disagree about incrementing the version here; the compliance rules would make 1.0.1 (or whichever) LRSs unable to accept 1.0.0 statements using account; given how early we caught it, I think avoiding that is a reasonable choice. This isn't an API that was deployed and used, this was a spec that was published (and I know no one was fully compliant yet when we caught the bug).

@dnjohnson
Copy link
Contributor

This might be a very rare exception since the adoption rate is low and that's why I'm not going to argue for it.

However, that scenario can still occur because some people might be operating from the published 1.0.0 PDF (or other reproductions) so you'll end up with a bunch of 1.0.0 statements being rejected by other 1.0.0 LRS'. I'd say that's significantly less favourable than explicitly indicating there's been a change in the spec.

@andyjohnson
Copy link
Contributor Author

I really don't think this is an issue as homepage (caps agnostic) was used 9 times in the document. Only one of these used a lower-case "p". The outlier was a typo and needed to be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants