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

Remove redundant tokenize code #112

Merged
merged 2 commits into from
Apr 27, 2017
Merged

Remove redundant tokenize code #112

merged 2 commits into from
Apr 27, 2017

Conversation

jpwhite4
Copy link
Member

Description

This change removes the unnecessary duplicate tokenize code. It also
replaces the function with a much simpler one that has full unit test
coverage.

Motivation and Context

General cleanup in preparation for deep linking support.

Tests performed

Ran the javascript unit_tests to confirm that the location hashes are parsed correctly. Ran code coverage
tools to confirm the new code has 100% test coverage (although the tools cannot check that all possible states have been hit in the regex state machine).

This change removes the unecessary duplicate tokenize code. It also
replaces the function with a much simpler one that has full unit test
coverage.
@plessbd
Copy link
Contributor

plessbd commented Apr 26, 2017

did you do some basic before and after tests to show that the same input got the same output with old and new code?

@plessbd
Copy link
Contributor

plessbd commented Apr 26, 2017

Will this new tokenize code break any existing code since this one always has the keys in the result?

var token = XDMoD.Dashboard.tokenize(document.location.hash);

var item = decodeURIComponent(CCR.tokenize(Ext.History.getToken()).params);

var token = CCR.xdmod.ui.Viewer.viewerInstance.tokenize(hash);

etc

https:/ubccr/xdmod/search?utf8=%E2%9C%93&q=tokenize&type=

@jpwhite4
Copy link
Member Author

To answer your questions: 1) Yes I did test , 2) No it won't break anything.

I note that the XDMoD.Dashboard.tokenize function was not modified in any way so this code change does not impact any code that calls it.

@plessbd
Copy link
Contributor

plessbd commented Apr 26, 2017

Screenshots or it didnt happen? :)

Looks Good To Me

@jpwhite4 jpwhite4 merged commit 161dea7 into ubccr:xdmod7.0 Apr 27, 2017
@jpwhite4 jpwhite4 deleted the tokenize branch April 27, 2017 18:42
@tyearke tyearke added enhancement Enhancement of the functionality of an existing feature qa labels May 10, 2017
@tyearke tyearke added this to the v7.0.0 milestone May 10, 2017
@plessbd plessbd added the qa / testing Updates/additions to tests label Mar 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of the functionality of an existing feature qa / testing Updates/additions to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants