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

File searching bug #1678

Merged
merged 14 commits into from
Sep 27, 2023
Merged

File searching bug #1678

merged 14 commits into from
Sep 27, 2023

Conversation

kbwestfall
Copy link
Collaborator

This does a few minor things:

  • Brings develop up to date with release after the 1.14.0 tag
  • Fixes a bug in the error message when the code finds more than one file with the same name (but presumably different extensions).
  • I'd like us to deprecate use of CHANGES and instead directly add to new "release" docs. Each time we do a release, I consolidate the listed changes into a doc and add it to our "What's New" page. I'm asking that we do that as we go now. If your change doesn't really fit under one of the existing headings, please add a new one. And, for each change, provide a narrative sentence or two explaining the change in a way that other developers (and eventually users) can understand. Other suggestions welcome.

@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2023

Codecov Report

Merging #1678 (2b6db67) into develop (287f682) will increase coverage by 0.01%.
The diff coverage is 20.51%.

❗ Current head 2b6db67 differs from pull request most recent head 1ab1ef0. Consider uploading reports for the commit 1ab1ef0 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@             Coverage Diff             @@
##           develop    #1678      +/-   ##
===========================================
+ Coverage    40.96%   40.98%   +0.01%     
===========================================
  Files          189      189              
  Lines        43684    43667      -17     
===========================================
  Hits         17896    17896              
+ Misses       25788    25771      -17     
Files Coverage Δ
pypeit/spectrographs/jwst_nirspec.py 30.92% <25.00%> (+0.62%) ⬆️
pypeit/spectrographs/keck_lris.py 13.25% <25.00%> (+0.03%) ⬆️
pypeit/spectrographs/lbt_mods.py 28.22% <25.00%> (+0.19%) ⬆️
pypeit/spectrographs/mmt_binospec.py 14.72% <25.00%> (+0.14%) ⬆️
pypeit/spectrographs/mmt_bluechannel.py 17.58% <25.00%> (+0.19%) ⬆️
pypeit/spectrographs/mmt_mmirs.py 20.00% <20.00%> (+0.24%) ⬆️
pypeit/spectrographs/keck_kcwi.py 30.07% <14.28%> (+0.22%) ⬆️
pypeit/utils.py 34.78% <14.28%> (+0.05%) ⬆️

Copy link
Collaborator

@tbowers7 tbowers7 left a comment

Choose a reason for hiding this comment

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

Great stuff. One minor suggestion about adding to utils.find_single_file(), but approving now.

CHANGES.rst Outdated
Comment on lines 5 to 10
PLEASE ADD TO doc/releases/1.14.1dev.rst!


1.14.0 (18 Sep 2023)
--------------------

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a really great idea -- and decreases the workload associated with tagging a release.

pypeit/spectrographs/jwst_nirspec.py Outdated Show resolved Hide resolved
Comment on lines +5 to +24
Dependency Changes
------------------

Functionality/Performance Improvements and Additions
----------------------------------------------------

Instrument-specific Updates
---------------------------

Script Changes
--------------

Datamodel Changes
-----------------

Under-the-hood Improvements
---------------------------

Bug Fixes
---------
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having all of these headings for the developer to use is very helpful (rather than the semi-chronological order in CHANGES.rst).

pypeit/utils.py Outdated Show resolved Hide resolved
@kbwestfall
Copy link
Collaborator Author

Following up on the call yesterday, I've updated the dev doc and added a section discussing how to do change logging now:

Logging changes
~~~~~~~~~~~~~~~

It is important to log changes made to the code in a way that other developers
and eventually users can interpret.  In the past we have done this using the
single ``CHANGES.rst`` file; however, we now have version specific change logs
in the ``doc/releases`` directory.  In terms of development guidelines:

- Changes made to the code should be logged in the relevant development log.
  For example, all changes made *after* version 1.14.0 will be logged in a
  ``doc/release/1.14.1dev.rst`` file.  If the relevant file doesn't exist when
  you submit your PR, create it.

- Changes are expected to fall under a small set of broad categories, like
  improvements to performance for specific instruments, minor bug fixes, or
  datamodel changes (see previous release docs for examples).  When including
  your change, add it below the relevant heading; if no relevant heading
  exists, add a new one.

- Hotfixes merged directly to the ``release`` branch should *also be added to
  the relevant development log*.  I.e., these changes are not part of the
  released tag, even if they are in the "release" branch.  Again, if the
  relevant file doesn't exist when you perform the hotfix, create it in a way
  that it will get merged with the identical doc in the ``develop`` branch.

- When tagging, the development log will be renamed to the new tag version, and
  a new file should be created for the next development phase.  See
  :ref:`tagging`.

Let me know if you have any questions or additions.

Copy link
Collaborator

@rcooke-ast rcooke-ast left a comment

Choose a reason for hiding this comment

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

All good from me - just some minor comments to think about, and then merge when you're ready!

1.14.1dev
---------

PLEASE ADD TO doc/releases/1.14.1dev.rst!
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great idea!

doc/dev/development.rst Show resolved Hide resolved
pypeit/spectrographs/keck_kcwi.py Outdated Show resolved Hide resolved
@rcooke-ast rcooke-ast mentioned this pull request Sep 27, 2023
@kbwestfall kbwestfall merged commit a79e2b2 into develop Sep 27, 2023
23 checks passed
@kbwestfall kbwestfall deleted the filesearch branch September 27, 2023 15:32
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.

4 participants