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

Improve finding date range in filenames (enforces separators) #1145

Merged
merged 28 commits into from
Jun 7, 2021

Conversation

senesis
Copy link
Contributor

@senesis senesis commented May 24, 2021

Description

Closes #1127

Function get_start_end_year(filename) has flaws, as described in #1127
The change makes use of regexp to better match date range syntax in filenames.

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.


To help with the number pull requests:

@senesis senesis marked this pull request as ready for review May 24, 2021 17:40
@senesis senesis requested review from zklaus and bsolino May 24, 2021 17:42
@senesis senesis self-assigned this May 24, 2021
@senesis senesis added the bug Something isn't working label May 24, 2021
@senesis senesis added this to the v2.3.0 milestone May 24, 2021
@senesis
Copy link
Contributor Author

senesis commented May 24, 2021

Note : this is a fast fix for V3.2.0, the proposal in #1078 should bring a more comprehensive solution, albeit probably later.

Copy link

@zklaus zklaus left a comment

Choose a reason for hiding this comment

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

Nice work. Minor comments.

@senesis senesis requested a review from zklaus May 26, 2021 14:22
@zklaus
Copy link

zklaus commented May 27, 2021

Oh I forgot to comment on the list of authors. This definitely grounds for adding your name. Go ahead!

@senesis senesis requested a review from zklaus May 27, 2021 21:21
@senesis
Copy link
Contributor Author

senesis commented May 28, 2021

@zklaus : I do not understand the codacy status for this PR : there were issues in two old commits but there is no more yet. Can you explain ?

@zklaus
Copy link

zklaus commented May 28, 2021

@senesis, sorry about the codacy thing. I think this happened because yesterday we renamed the master to main. Don't worry about it.

One last thing: You removed a bunch of information from the .zenodo.json file. Was this intentional?

@senesis
Copy link
Contributor Author

senesis commented May 28, 2021

For the zenodo.json file, I applied the procedure described in the doc, noticed the large changes, and assumed that this was due to a change in what the procedure is supposed to produce. Would you like me to restore the discarded part ?

@senesis
Copy link
Contributor Author

senesis commented May 28, 2021

@zklaus : The list of commits for this PR is definitely much longer than suitable. Is there a way to aggregate them in a single one without creating a new branch and a new PR ?

@zklaus
Copy link

zklaus commented May 28, 2021

On the list of commits, let me first point out that we squash on merge. That means, no matter what happens in this branch and PR, when it is merged into master it will be put in as a single commit and that is the only way how it will appear in the history from that point on. So maybe in light of this, you are ok with the state of things? I personally wouldn't worry about it.

On the zenodo file, I am not sure, to be honest. @bouweandela, or perhaps @nielsdrost, could you please weigh in? It seems the way we instruct people to add themselves to the author list, we lose project and grant information from our zenodo record. Is this correct?

@senesis
Copy link
Contributor Author

senesis commented May 28, 2021

OK re. the list of commits. I expected that this squeeze occurs

@bouweandela
Copy link
Member

For the zenodo.json file, I applied the procedure described in the doc, noticed the large changes, and assumed that this was due to a change in what the procedure is supposed to produce. Would you like me to restore the discarded part ?

Yes, please! If you have the time, it would be great if you could also add the omission in the documentation. This ifnromation is used to fill the Grants and Communities sections that you see listed on the right hand side on our zenodo page: https://zenodo.org/record/4525749

@senesis
Copy link
Contributor Author

senesis commented May 29, 2021

Hello @bouweandela and @zklaus
I think everything is OK now with that PR

@senesis senesis removed the request for review from bsolino June 7, 2021 13:03
@zklaus zklaus merged commit 5a8ad83 into main Jun 7, 2021
@zklaus zklaus deleted the Improve_date_range_regexp branch June 7, 2021 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In filenames, experiments names which include a date do mislead recognizing file time period
3 participants