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

Quickfix: update naming of docstring field within codebase to match API #320

Merged
merged 1 commit into from
Nov 27, 2020

Conversation

studioswong
Copy link
Contributor

Description

While going through the codebase I realize that the docstring field within our codebase was named under JS camel case, which we should follow the python naming within our codebase.

Development notes

I have changed the naming of the field from docString to docstring within our store, reducers and tests.

QA notes

N/A

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

Legal notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

  • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.

  • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.

  • I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

Copy link
Contributor

@richardwestenra richardwestenra left a comment

Choose a reason for hiding this comment

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

Sounds good - is this the case used in the API output JSON? Just want to doublecheck that we're matching up here.

@studioswong
Copy link
Contributor Author

Thanks for raising this @richardwestenra ! For the API output JSON, do you mean the case of running viz with the JSON data file? This change wouldn't affect JSON API output, given that this only refers to the data returned from the nodes endpoint from a running Kedro project and would only need to refer to the data format returned from the endpoint (which, on implementing ticket(KED-2309), viz would simply not fire any API calls on selecting a node).

Speaking of, on running viz from a JSON data file, what will the metadata panel show for the dynamic data fields that should be fetched from the API? @bru5

@studioswong studioswong merged commit f774eb4 into main Nov 27, 2020
@studioswong studioswong deleted the fix/fix-naming-of-docstring branch November 27, 2020 20:09
@richardwestenra
Copy link
Contributor

No I meant 'case' as in camelCase vs snake_case vs lowercase vs UPPERCASE. So I was asking, what case is the API output in? Because we should be matching that in normalize-data.js when reading it, even if it's not case-sensitive. e.g. if it's in camelCase then we should do something like

state.node.docstring[id] = node.docString;

@richardwestenra
Copy link
Contributor

Update: I just manually checked and yeah it's all lowercase in the API output 👍

as you were 😄

@richardwestenra richardwestenra mentioned this pull request Dec 16, 2020
1 task
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.

3 participants