Placeholder pull request for project-wide code review #30
Conversation
Getting synced with my latest; will use ets authoritative going forward
Catching up to latest changes
Helping Elastic Beanstalk get the paths right to find js & css files
Removing "temporary code to debug the elastic beanstalk run context"
…input before being sent to gcp
Leading and trailing blank space are now removed from user gs:// url …
Clear contents of input, output fields when clicking back into input field
| source venv/bin/activate | ||
| export HARTSFIELD_LOCAL_CONFIGS=/Users/gregm/rip_hartsfield/hartsfield_config | ||
| source .env.development | ||
| export HARTSFIELD_LOCAL_CONFIGS=/Volumes/XYZ/hartsfield_config | ||
| export HARTSFIELD_ENV=development | ||
| npm run serve-vue | ||
| ``` |
There was a problem hiding this comment.
source .env.development seems unnecessary in both instances above. Vite should pick it up automatically when npm run serve-vue
| if not re.match(r'gs://.{3,}/.+', gs_source_url): | ||
| error_message = 'The submitted data \"' + gs_source_url + '\" is not a valid gs_source_url.' | ||
| v = {'response': error_message, 'status': 'error'} | ||
| return tolerant_jsonify(v) |
There was a problem hiding this comment.
The above looks good. Although I suggest the following for line 58:
return tolerant_jsonify(
{'message': error_message},
500,
)
This will make for more standard front-end code (eg, checking HTTP status code).
| error_message = 'There was an exception trying to do the GCP storage operation with the submitted data \"' + \ | ||
| gs_source_url + \ | ||
| '\'. When GCP tried, it told us: \"' + \ | ||
| str(e) + '\"' |
There was a problem hiding this comment.
Minor code style concern: Use f-string with triple quotes when string is multi-line.
m = f"""
Hello world. My name is {name}.
"""
hartsfield/models/user.py
Outdated
| 'id': uid, | ||
| 'isActive': is_active, | ||
| 'isAdmin': is_active, | ||
| 'isAnonymous': not is_active, |
There was a problem hiding this comment.
I think this line should be: 'isAnonymous': not uid. An inactive user with a UID is not anonymous.
| "eslint-plugin-vue-scoped-css": "^2.5.1", | ||
| "sass": "^1.69.5", | ||
| "typescript": "5.2.2", | ||
| "vite": "^4.5.1", |
There was a problem hiding this comment.
Fyi, the latest vite is v5.*
settings.json
Outdated
| }, | ||
| "javascript.suggest.includeAutomaticOptionalChainCompletions": false, | ||
| "debug.saveBeforeStart": "none" | ||
| } |
There was a problem hiding this comment.
Minor: Files should have trailing line breaks so we don't get these annoying little warning symbols. 😛
src/api/fetch-url.ts
Outdated
|
|
||
| export function fetchUrl(gs_source_url: string) { | ||
|
|
||
| return utils.post('/api/fetch_url_direct', {gs_source_url}, true) |
There was a problem hiding this comment.
Minor code style: we typically use camelCase when posting query args.
src/components/utils/Header1.vue
Outdated
| } | ||
| } | ||
| } | ||
| </script> |
There was a problem hiding this comment.
Indentation fix needed above. (Would the front-end linter catch this?)
There was a problem hiding this comment.
Would love for the linter to fix this for me! I guess the way I used it, it did not catch this? gmerritt/hartsfield-from-ripley@a44aa2a
There was a problem hiding this comment.
@gmerritt - Maybe this eslint rule will make the check and fix?! https://eslint.vuejs.org/rules/script-indent
| }, | ||
| methods: { | ||
| ...mapActions(useContextStore, [ | ||
| 'alertScreenReader', | ||
| 'loadingComplete', | ||
| 'loadingStart' | ||
| ]) |
There was a problem hiding this comment.
Having this "mixin" makes sense and most of our other tools follow this pattern. However, in the Vue 3 world, mixins are discouraged. I don't think you should refactor (yet). Maybe down the road. 😄 See https://vuejs.org/api/options-composition.html
| VSwitch, | ||
| VTextarea, | ||
| VTextField, | ||
| VTooltip |
There was a problem hiding this comment.
As probably you know, the list above should be limited to components actually used by Hartsfield. Perhaps we trim the list when all Hartsfield features are implemented.
src/stores/index.ts
Outdated
| @@ -1,4 +1,7 @@ | |||
| import { useContextStore } from '@/stores/context' | |||
| import Vuex from 'vuex' | |||
|
|
|||
| Vue.use(Vuex) | |||
There was a problem hiding this comment.
Hartsfield uses Pinia and not Vuex, right? The lines above should be dropped.
| placeholder="gs://" | ||
| /> | ||
| </v-col> | ||
| <div v-if="!currentUser" class="pt-2 page-fetch-url"> |
There was a problem hiding this comment.
currentUser should NEVER be null. Thus, !currentUser will always be false. Do we want !currentUser.isAuthenticated
|
|
||
|
|
||
|
|
||
| </div> |
There was a problem hiding this comment.
Your call on front-end code style. I prefer a two-space indentation, enforced by the linter. And one empty-line max.
There was a problem hiding this comment.
[comment as above re: linter] -- Would love for the linter to fix this for me! I guess the way I used it, it did not catch this? gmerritt/hartsfield-from-ripley@a44aa2a
There was a problem hiding this comment.
ohhh maybe I'm needing 'indent': ['error', 2], in my .eslintrc.js!
| font-weight: 400; | ||
| } | ||
| } | ||
| </style> |
There was a problem hiding this comment.
Since you're building this Vue 3 app from scratch, you can use Vue's Composition API. DevOps is still behind the curve in this respect because we're most familiar with the old "Options API" syntax. Our two Vue3 apps (BOA and Ripley) were migrated from Vue 2 and inherited a bunch of the old syntax. For the record, I do find the Composition API cleaner.
|
@gmerritt @pauline2k - My code review is done. Let me know if any of my comments are unclear. Thanks! |
|
Super cool, thank you for all of the help @johncrossman! |
Formatting clean-ups from John C.'s feedback
Additional clean-ups from John C.'s feedback
See #29 for some candidate areas of interest.