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

Update Merlin IDF and IPF #24403

Merged
merged 7 commits into from Jan 9, 2019
Merged

Update Merlin IDF and IPF #24403

merged 7 commits into from Jan 9, 2019

Conversation

abuts
Copy link
Member

@abuts abuts commented Jan 3, 2019

Description of work.

Merlin monitors have been modified and the ticket reflects these changes.
Have no idea what to test here --- there are no code changes here and the changes to IDF-s are trivial.

Code Review
  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards?
  • Are the unit tests small and test the class in isolation?
  • If there are changes in the release notes then do they describe the changes appropriately?
Functional Tests
  • Do changes function as described? Add comments below that describe the tests performed?
  • Do the changes handle unexpected situations, e.g. bad input?
  • Has the relevant (user and developer) documentation been added/updated?

Does everything look good? Mark the review as Approve. A member of @mantidproject/gatekeepers will take care of it.

@abuts abuts added Direct Inelastic Issues and pull requests related to direct inelastic Misc: Easy labels Jan 3, 2019
@@ -0,0 +1,478 @@
<?xml version="1.0" encoding="UTF-8" ?>
<parameter-file instrument = "MERLIN" valid-from = "2017-09-12T11:00:01">
Copy link
Member

Choose a reason for hiding this comment

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

Is the valid-from date correct here? It does not match the year indicated by the filename.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair point. I have not modified it as, AFAIK, parameters file date is not used anywhere, but it may be used in a future. I have modified it making equal to Defenition date.

@DanNixon DanNixon self-assigned this Jan 4, 2019
Copy link
Member

@DanNixon DanNixon left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Can you improve the commit message of 46b1a6b to describe what you have actually done (and that the "issue" is/was) and update the PR title.

@abuts
Copy link
Member Author

abuts commented Jan 4, 2019

@DanNixon I do not know how to edit the commit message unless reverting and rebasing, but this is a bit excessive to do full rebase for this kind of message. How can I edit commit message only?

The issue is mentioned in the ticket -- the Merlin physical and logical monitors have changed so appropriate software changes are necessary.

@DanNixon
Copy link
Member

DanNixon commented Jan 4, 2019

Rebaseing would be the correct method here. It is not excessive to correct a commit message that gives no information whatsoever about the changes within the commit.

@abuts
Copy link
Member Author

abuts commented Jan 4, 2019

@DanNixon Could you, please, provide me with the step-by step algorithm of the rebasing. I have not done it for a while and have different tasks to do and you seems have enough time to do everything right.

@DanNixon
Copy link
Member

DanNixon commented Jan 4, 2019

https://help.github.com/articles/changing-a-commit-message/ pretty much describes it under the section "Amending the message of older or multiple commit messages". Note that you should use --force-with-lease instead of --force.

@abuts abuts dismissed DanNixon’s stale review January 4, 2019 16:25

The code quality is not affected or improvement is not worth efforts which is confirmed by the fact that the reviewer is not ready to provide me with the instructions on making the necessary improvements

@abuts
Copy link
Member Author

abuts commented Jan 6, 2019

Well, it was not difficult to do such change though not sure any improvement was achieved.

@soininen soininen changed the title Re #24400 Should fix the issue Update Merlin IDF and IPF Jan 9, 2019
Copy link

@soininen soininen left a comment

Choose a reason for hiding this comment

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

Looks good but I think the changes should be mentioned in the release notes, no?

@abuts
Copy link
Member Author

abuts commented Jan 9, 2019

@soininen -- its so minor that not worth mentioned in release notes. The instrument scientists know that the changes have been applied (They have requested it and I've informed them about the implementation) and nobody else should be bothered.
Current Mantid workflow will update these files on local machines as soon as they merged to master so its not an release issue.

@soininen
Copy link

soininen commented Jan 9, 2019

@abuts I see, I guess it is OK then. This could have been mentioned in the PR's description.

@peterfpeterson peterfpeterson merged commit 77f88e8 into master Jan 9, 2019
@peterfpeterson peterfpeterson deleted the 24400_new_MERLIN_IDF branch January 9, 2019 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Direct Inelastic Issues and pull requests related to direct inelastic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants