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

Updating README #71

Merged
merged 25 commits into from
Jun 21, 2022
Merged

Updating README #71

merged 25 commits into from
Jun 21, 2022

Conversation

muhanadz
Copy link
Member

@muhanadz muhanadz commented Apr 7, 2022

Updating README.rst to include the changes to the default behavior from #67.

Updating README.rst to include the changes to default behaviour
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
Fixed grammer. Added more examples for automatic header detection. Added linkable titles to some headings
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

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

A few remaining rst linting errors to fix. Two outstanding questions:

  • from the UoD team, are there any objections to adopting the new behavior both in the context of IDR as well as the training? /cc @dominikl @pwalczysko
  • should we get the new feature released as 0.10.0 or is it time to call this 1.0.0? If the latter was desirable but we felt like we need additional evaluation, an intermediate approach would be to get this as a pre-releases e.g. 1.0.0a1.

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
@sbesson sbesson self-requested a review April 15, 2022 15:16
@sbesson sbesson dismissed their stale review April 15, 2022 15:16

RST fixes pushed

@sbesson
Copy link
Member

sbesson commented Apr 15, 2022

Thanks @muhanadz. This is now staged for review at https://github.com/ome/omero-metadata/blob/0cc977f9c4d2309b8712f2ef20ad93cba68295f2/README.rst. As mentioned above, next step is to get a sign off from the team and decide on the versioning/roadmap.

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
Co-authored-by: pwalczysko <[email protected]>
README.rst Outdated Show resolved Hide resolved
@pwalczysko
Copy link
Member

Tested the examples listed for the automatic column detection (Project, Dataset, Screen and Image (adding roi table)). Worked fine. Happy to test the new @muhanadz 's examples too.

@sbesson sbesson requested a review from pwalczysko May 12, 2022 11:22
@sbesson sbesson requested review from will-moore and pwalczysko and removed request for dominikl May 25, 2022 19:45
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
@pwalczysko
Copy link
Member

Couple of suggestions, but looks good otherwise.

README.rst Outdated Show resolved Hide resolved
@will-moore
Copy link
Member

Final read-through, looking good - couple of comments.
Also just noticed Copyright: "2018-2021, The Open Microscopy Environment" could be updated.

@will-moore will-moore dismissed their stale review May 31, 2022 10:00

Looks good

@muhanadz
Copy link
Member Author

muhanadz commented Jun 1, 2022

Added final changes from comments and suggestions (thank you all). Made some changes to the object-types table to address the conversation here: #71 (comment)

Copy link
Member

@will-moore will-moore left a comment

Choose a reason for hiding this comment

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

All looks good now, thanks!

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

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

Also happy for this to go in and I'll leave @pwalczysko to submit his review.

Once this is in, #77 is the only PR I would propose for review and possible inclusion in 0.11.0 as it fixes a specific (but not uncommon) use case related to the new detection feature. If there is no reviewing capacity, this can be scheduled for a patch release.

@sbesson sbesson added this to the 0.11.0 milestone Jun 10, 2022
README.rst Outdated Show resolved Hide resolved
@pwalczysko
Copy link
Member

Sorry for the delay. Like the text, just one missing dot https://github.com/ome/omero-metadata/pull/71/files#r902463366 spotted. After accepting the suggestion https://github.com/ome/omero-metadata/pull/71/files#r902463366 all looks good to me here.

Copy link
Member

@pwalczysko pwalczysko left a comment

Choose a reason for hiding this comment

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

Co-authored-by: pwalczysko <[email protected]>
@sbesson
Copy link
Member

sbesson commented Jun 21, 2022

Thanks all for the time spent on this readme. Lots of back n forth but i think in addition to introducing the new detection, the readme now clarifies several behaviors of the plugin.

From my side the only blocker to releasing 0.11.0 is the scenario of csv files with sparse numerical data which is currently broken when using the default command (see #76). #77 proposes a fix for this regression by adding a new parameter in the HeaderDetection API. @will-moore @muhanadz could you take a look at this and see if the proposal makes sense?

@sbesson sbesson merged commit f7d5c91 into ome:master Jun 21, 2022
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