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

PascalCase files #6955

Merged
merged 13 commits into from
Sep 14, 2023
Merged

PascalCase files #6955

merged 13 commits into from
Sep 14, 2023

Conversation

depperm
Copy link
Contributor

@depperm depperm commented Aug 18, 2023

Closes #5216

Describe your changes:

Added linting rule for files to be pascal case as referenced.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

  • Changes address original issue?
  • Tests included and/or updated with changes?
  • Command line build passes?
  • Has this been smoke tested?
  • Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

  • Changes appear to address issue?
  • Reviewer has tested changes by following the provided instructions?
  • Changes appear not to be breaking changes?
  • Appropriate automated tests included?
  • Code style and in-line documentation are appropriate?
  • Has associated issue been labelled unverified? (only applicable if this PR closes the issue)
  • Has associated issue been labelled bug? (only applicable if this PR is for a bug fix)

@ozyx ozyx added the source:community Community contribution or request label Aug 18, 2023
@depperm depperm mentioned this pull request Aug 19, 2023
15 tasks
@ozyx ozyx self-requested a review August 21, 2023 16:53
@ozyx
Copy link
Member

ozyx commented Aug 21, 2023

Thanks for this! Could you please fix the failing tests? Let us know if you have any questions.

@depperm
Copy link
Contributor Author

depperm commented Aug 23, 2023

@ozyx I can't reproduce the error that circleci is showing. Am I doing something wrong?
image

@ozyx
Copy link
Member

ozyx commented Aug 23, 2023

@ozyx I can't reproduce the error that circleci is showing. Am I doing something wrong?

image

No, I think circleci is cacheing something. Will look into it today. The tests seem to pass locally and all looks good!

@unlikelyzero
Copy link
Collaborator

@ozyx @depperm please let me know if this is clear: #6988

@depperm
Copy link
Contributor Author

depperm commented Aug 24, 2023

@ozyx @depperm please let me know if this is clear: #6988

I don't think I can schedule a re-run of tests. Clicking trigger pipeline doesn't do anything

@ozyx
Copy link
Member

ozyx commented Aug 24, 2023

@ozyx @depperm please let me know if this is clear: #6988

I don't think I can schedule a re-run of tests. Clicking trigger pipeline doesn't do anything

I think only members of the @nasa organization are able to trigger builds manually. However, I did trigger a build using the instructions provided and I'm seeing the same errors, so something else is going on here @unlikelyzero .

Will investigate today and keep you posted @depperm

@ozyx
Copy link
Member

ozyx commented Aug 24, 2023

@depperm OK.

It looks like some files were not renamed. Take a look at src/plugins/LADTable. We need to be careful here. When renaming files, be sure to use git mv so that git picks it up as a file rename. We should be okay, but there's a chance that a change like this can lose git commit history, especially given that we squash commits.

Please ensure that all files have been renamed properly and let's try again.

@depperm
Copy link
Contributor Author

depperm commented Aug 25, 2023

@ozyx

It looks like some files were not renamed. Take a look at src/plugins/LADTable. We need to be careful here. When renaming files, be sure to use git mv so that git picks it up as a file rename. We should be okay, but there's a chance that a change like this can lose git commit history, especially given that we squash commits.

Do you mean the directory name? I was under the impression this was only for Vue components not all file/dir naming convention. I can change all files. Is this the case for all naming (all .js files and directories)?

@ozyx
Copy link
Member

ozyx commented Aug 25, 2023

@ozyx

It looks like some files were not renamed. Take a look at src/plugins/LADTable. We need to be careful here. When renaming files, be sure to use git mv so that git picks it up as a file rename. We should be okay, but there's a chance that a change like this can lose git commit history, especially given that we squash commits.

Do you mean the directory name? I was under the impression this was only for Vue components not all file/dir naming convention. I can change all files. Is this the case for all naming (all .js files and directories)?

Sorry, let me clarify.

Here we reference LadTable.vue.

But here you can see that the file was never renamed, it's still LADTable.vue. Hence the failures when we try to bundle with webpack.

@deploysentinel
Copy link

deploysentinel bot commented Aug 25, 2023

Current Playwright Test Results Summary

✅ 138 Passing - ⚠️ 1 Flaky

Run may still be in progress, this comment will be updated as current testing workflow or job completes...

(Last updated on 09/14/2023 12:52:45am UTC)

Run Details

Running Job e2e-stable on CircleCI

Commit: c14773e

Started: 09/14/2023 12:46:12am UTC

⚠️ Flakes

📄   functional/plugins/plot/logPlot.e2e.spec.js • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Log plot tests Log Plot ticks are functionally correct in regular and log mode and after refresh
Retry 1Initial Attempt
2.38% (1) 1 / 42 run
failed over last 7 days
33.33% (14) 14 / 42 runs
flaked over last 7 days

View Detailed Build Results


@codecov
Copy link

codecov bot commented Aug 25, 2023

Codecov Report

Merging #6955 (c14773e) into master (95ac919) will decrease coverage by 0.30%.
The diff coverage is 27.58%.

@@            Coverage Diff             @@
##           master    #6955      +/-   ##
==========================================
- Coverage   55.78%   55.48%   -0.30%     
==========================================
  Files         643      643              
  Lines       25787    25801      +14     
  Branches     2517     2517              
==========================================
- Hits        14385    14316      -69     
- Misses      10706    10797      +91     
+ Partials      696      688       -8     
Flag Coverage Δ *Carryforward flag
e2e-full 42.05% <ø> (+<0.01%) ⬆️ Carriedforward from f668651
e2e-stable 57.22% <ø> (-0.02%) ⬇️
unit 48.98% <27.58%> (-0.44%) ⬇️

*This pull request uses carry forward flags. Click here to find out more.

Files Changed Coverage Δ
src/api/forms/components/controls/Locator.vue 0.00% <ø> (ø)
...gins/LADTable/LADTableConfigurationViewProvider.js 83.33% <ø> (ø)
src/plugins/LADTable/LADTableView.js 93.33% <ø> (ø)
src/plugins/LADTable/components/LadRow.vue 53.40% <ø> (ø)
src/plugins/LADTable/components/LadTable.vue 40.00% <0.00%> (ø)
...gins/LADTable/components/LadTableConfiguration.vue 0.00% <ø> (ø)
src/plugins/LADTable/components/LadTableSet.vue 45.54% <ø> (ø)
...gins/clearData/components/GlobalClearIndicator.vue 100.00% <ø> (ø)
src/plugins/clearData/plugin.js 100.00% <ø> (ø)
...ins/condition/components/inspector/StyleEditor.vue 51.85% <ø> (ø)
... and 46 more

... and 15 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95ac919...c14773e. Read the comment docs.

@ozyx
Copy link
Member

ozyx commented Sep 13, 2023

Hi @depperm,

Sorry for the delay. We intend on merging this, but will need to verify that it won't impact other open PRs (difficult merge conflicts). In the past, we've had to do some of the more extensive refactorings (such as prettier) in stages. I need to determine if we should do the same for this PR as well.

Thanks for your patience!

@ozyx
Copy link
Member

ozyx commented Sep 13, 2023

@depperm Could you please fix the conflict on your local repo? It seems I'm unable to fix it on the remote for some reason.

@depperm
Copy link
Contributor Author

depperm commented Sep 13, 2023

@ozyx merged

@ozyx ozyx added the pr:e2e:couchdb npm run test:e2e:couchdb label Sep 14, 2023
@ozyx ozyx added this to the Target:3.1.0 milestone Sep 14, 2023
Copy link
Member

@ozyx ozyx left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@ozyx ozyx merged commit 6776cc3 into nasa:master Sep 14, 2023
17 of 18 checks passed
@shefalijoshi shefalijoshi added the type:maintenance tests, chores, or project maintenance label Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:e2e:couchdb npm run test:e2e:couchdb source:community Community contribution or request type:maintenance tests, chores, or project maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Linting] Fix casesensitive filename imports once and for all
4 participants