search

bcgov / DBC-APIM

DataBC OPEN API Services
Apache License 2.0
1 stars 0 forks source link

code review for ckanext-openapiviewer/issues/20 #37

Closed ll911 closed 7 years ago

ll911 commented 7 years ago

https://github.com/bcgov/ckanext-openapiviewer/issues/20

banders commented 7 years ago

I have looked through the code review results for swagger-ui. None of the "bugs" appear to be actual problems. 34 of 35 "bugs" are supposedly due to invalid values for "font-family" in css. Unfortunately the code review doesn't tell us specifically where in the file the error occurs (because the file is minified), so I have looked at all occurrences of "font-family" in the source code and all the values seem reasonable and correct. I also checked a few elements affected by the font-family rules to confirm that in those cases Chrome/Firefox/IE are displaying the expected font... they are displaying the expected font.

There is one other swagger-ui "bug" which is legitimate ('font-style: bold' should instead by 'font-weight: bold'), but that css problem affects a class that seem to not actually be used in the code. Nevertheless I have corrected the issue in this branch.

There are thousands of "code smells" affecting swagger-ui. Most of the "major" code smells relate to "over-specified selector". These are a difficult ones to fix because most swagger-ui files would be impacted and it would take substantial testing across multiple browsers to confirm that everything continues to display as expected. In my opinion an "over-specified selector" is a minor issues which doesn't affect any functionality and it doesn't warrant any extensive efforts to change. In addition, I doubt the swagger-ui community is going to accept a pull request that affects hundreds of lines across many files to fix a minor issue. I suggest we take no action on the "over-specified selectors". There are many "minor" code smells related to whitespace in css files, which we can ignore because the css has been minified for faster load times.

I also looked through the "code smells" affecting the ckanext-openapiviewer python code. Again, most of the listed problems are bogus. For example, many of the listed problems suggest the indentation is invalid, which is untrue. There were a few legitimate "code smells" related to unneeded imports which I have fixed. Pull request for ckanext-openapiviewer is here.