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

[FIX] Documentation links #3897

Merged
merged 1 commit into from
Jun 28, 2019
Merged

Conversation

rokgomiscek
Copy link
Contributor

Issue

Some widgets reported missing documentation even if this wasn't the case.

Description of changes

Fixed names so documentation links point to the right file.

Includes
  • Code changes
  • Tests
  • Documentation

@@ -125,7 +125,7 @@ class State(enum.Enum):


class OWTestLearners(OWWidget):
name = "Test & Score"
name = "Test and Score"
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes the name of the widget in Orange. Which means all schemas that had "Test & Score" before could fail. Please check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works (I checked on an example workflow).

@@ -94,7 +94,7 @@ class NotEnoughData(ValueError):


class OWKMeans(widget.OWWidget):
name = "k-Means"
name = "k-Means clustering"
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes the name of the widget and makes it longer. I don't think this is ok. If docs don't match, we can change the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing the name would make this similar to Hierachical and Louvain Clustering (though "clustering" should be capitalized).

Copy link
Contributor

Choose a reason for hiding this comment

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

Hierarchical and Louvain can not be used by themselves - "clustering" is part of the name there.
k-Means is clear enough and can be used as a name by itself.
I think we should strive for short widget names, so I would not expand it in this case.
BTW, clustering is already a keyword for k-means so using it in a search lists it alongside other methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Changed back widget name and changed documentation file name.

@@ -1,4 +1,4 @@
Transform
Apply Domain
=========
Copy link
Contributor

Choose a reason for hiding this comment

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

Underscores have to fit the length of the title.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the change in not reflected in the index.rst. Could you please fix this too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for these continuous comments. The documentation for Apply Domain should be changed to reflect the changes in the widget name. Namely the references to the widget, image names (so we know what they refer to) and likely the example workflows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
Apply Domain (ex Transform) not present in example workflows.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I didn't mean example workflows in Orange tutorial section, but workflows that are in Example section of the documentation.

@@ -1,4 +1,4 @@
Predictions
Confusion Matrix
===========
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

@codecov
Copy link

codecov bot commented Jun 21, 2019

Codecov Report

Merging #3897 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #3897   +/-   ##
=======================================
  Coverage   84.29%   84.29%           
=======================================
  Files         384      384           
  Lines       72749    72749           
=======================================
  Hits        61327    61327           
  Misses      11422    11422

@ajdapretnar ajdapretnar merged commit f41f77b into biolab:master Jun 28, 2019
@rokgomiscek rokgomiscek deleted the documentation_fix branch June 28, 2019 14:07
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