Skip to content
This repository has been archived by the owner on Jun 16, 2018. It is now read-only.

Added the ability to control how score gets exported #262

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Added the ability to control how score gets exported #262

wants to merge 12 commits into from

Conversation

ThinkRedstone
Copy link

Again, this is above one commit from sorting icons; otherwise no teams are rendered in ranking.hrml
Currently, if a team has a null field (for example, did not participate in one round) that gets encoded as the string "null". If we don't want this behaviour, I can add a setting to replace null with an empty string or a user specified one.

Copy link
Collaborator

@poelstra poelstra left a comment

Choose a reason for hiding this comment

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

Your new export cannot produce 'proper' .csv files. For example, quoting of fields in case it contains the delimiter.

Note that the existing functionality allows to simply double-click the generated .csv file, to open it in Excel, even with 'complicated' team names etc.
Also note that it exports the header.

I understand that you like to make the export compatible to the program you already have, but please retain the existing functionality as well.

Making a generic export with completely custom delimiters is tricky, and still a lot of work for a user to configure correctly.

My recommendation therefore would be to replace all these detailed settings with a simple radio-button (or drop-down box), with just (for now) "CSV file", "TAB separated" and " compatible".

Each export can then decide on delimiters, quoting, whether or not to include a header, all scores or just high scores, etc.

Additionally, like the other PRs, please rebase on top of master and leave out the sorting changes.

Also, when you're e.g. going to 'undo' certain changes, please do not just introduce another commit that again changes these lines, but 'fixup' the original commit instead (see git rebase -i).


beforeEach(function() {
angular.mock.module(module.name);
angular.mock.inject(function($controller, $rootScope,$q) {
$scope = $rootScope.$new();
scoresMock = createScoresMock($q);
handshakeMock = createHandshakeMock($q);
stagesMock = createStagesMock();
stagesMock = createStagesMock($q);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not ideal to have to pass the promise implementation to the mock.
Just use Promise inside of it, and/or the 'normal' q package.

Copy link
Member

Choose a reason for hiding this comment

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

I believe this is legacy from our side. Not sure how to fix that as the stages services used $q to resolve promises in an "angular aware" fashion.

The other option would be to not use $q at all in the service, but wrap its use in $q.when() everywhere, but that seems even less ideal

*/
$scope.encodeArray = function (array) {
var string = "";
var settings = $settings.settings;
Copy link
Collaborator

Choose a reason for hiding this comment

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

encodeArray now 'depends' on $settings, making it harder to refactor out of here and re-use elsewhere, and harder to unit-test.
Better to explicitly pass the settings in.

entry.team.name,
entry.highest,
].concat(entry.scores);
$scope.buildExportFiles= function () {
Copy link
Collaborator

Choose a reason for hiding this comment

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

rebuildCSV(scoreboard) gets passed in the scoreboard. This is intentional, because it may be that someone wanted to pass in e.g. a filtered version of the scoreboard.

(It did directly tweak things on $scope, which is bad, so probably it should have returned an object containing these items instead.)

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

Successfully merging this pull request may close these issues.

3 participants